Antony Dovgal wrote: > On 03/01/2007 12:35 AM, Doug Goldstein wrote: >>> Did you really test it with non-NULL terminated strings? >>> Don't you need to add '\0' manually? >> >> The test is that you run the example code from bug #38819, watch PHP >> crash. Apply my patch and watch PHP not crash. Fairly simple. My >> backtrace >> is identical to the reporter's. > > Well, I can't do it myself since I don't even have a LDAP server > installed. > That's why I asked you the question. > >> If you read the comments by the OpenLDAP developers in the two bugs >> referenced they have the same reason for using ldap_get_values_len() >> instead of ldap_get_values() because it's safer incase the data is >> non-NULL terminated data. In this case PHP's assumption that it's NULL >> terminated is flawed since it's crashing since it's extending past the >> end >> of it's memory segment. (as visible from bug #38819) > > I have no doubts it's true, but the question was: > did you really test [the NEW patched version of] the code with non-NULL > terminated strings?
If I run the example PHP code from bug #38819, PHP will merrily run off the end of a string into no man's land and crash as per the backtrace in bug #38819. With the patch applied, it does not. That sound clearly like the example PHP code in bug #38819 is testing it with a non-NULL terminated string. I hope this is clear. > > As far as I understand, if OpenLDAP returns non-NULL terminated string, > we'd most likely need > to add the terminator, otherwise we'd end up with non-NULL terminated > strings in PHP. > Hence the question. > > Do I make myself clear enough? If you read the PHP API documentation, the API function call add_index_stringl() http://php.interec.es/manual/nl/zend-api.add-index-stringl.php Was created for this explicit purpose. There is no need to tack on a NULL character since it is automatically happening. If it's not happening, then it is properly using the size passed to it to always ensure it doesn't wander off the end of the memory address anywhere. I hope that is clear. > >>> No, replace means "add something and delete the original", you added >>> new >>> function and >>> commented out the original one, which made me wonder why you didn't >>> change >>> the original function instead. >>> What was the point in creating new function adding an alias? >>> There is nothing wrong, I'm just curious. >> >> I removed PHP_FE(ldap_get_values) because it's a pointless function. >> It's >> identical in code to PHP_FE(ldap_get_values_len) so I made it an alias. >> Less code to maintain for you guys. Less bloat. Everyone should be >> happy. > > Ok, so we got some misunderstanding here.. > >> I removed PHP_FE(ldap_get_values) because it's a pointless function. > > The point is that the patch should FIX this function, not REMOVE it. > And it's still there, but commented out (which makes not sense). > > Don't get me wrong, I can (and I will) rewrite it myself, but I just > wanted to understand what was the initial idea behind it. The implemented PHP_FUNCTION(ldap_get_values) and PHP_FUNCTION(ldap_get_values_len) differ on in the fact that they call ldap_get_values() and ldap_get_values_len(). Since the premise behind all these fixes is to remove the calls to ldap_get_values() and replace them with ldap_get_values_len() so you ensure that you don't wander off the end of a string and cause a crash.. that makes PHP_FUNCTION(ldap_get_values) and PHP_FUNCTION(ldap_get_values_len) identical in code. Line for line there would be 0 difference. Rather then having a completely duplicated function and duplicated code (duplicated code is bad), I changed PHP_FUNCTION(ldap_get_values) to an alias of PHP_FUNCTION(ldap_get_values_len) > > -- > Wbr, > Antony Dovgal > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > > -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
