Antony Dovgal wrote: > On 03/01/2007 12:04 AM, Doug Goldstein wrote: >>> Wait, I thought the DEPRECATED thing fixed it (I can't test it myself >>> as I >>> don't use LDAP). >>> If not, then what was it all about? >> >> The test case still failed for me. It also does not address the fact >> that >> the usage of the PHP ldap functions is unsafe because they could return >> a >> char * with a non-NULL terminated string and your code will eventually >> call strlen() on it. With the changes I have made it knows the length >> from >> the LDAP server and doesn't need to use strlen(). Since >> add_index_stringl() doesn't call strlen() since the length is provided. > > 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. 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) > >>> Some comments regarding the patch: >>> First of all, why do you add a new function and comment the old one >>> instead of >>> changing/fixing the old function? >>> We definitely don't need dead/commented out code in the sources =) >> >> I didn't add a new function. I replaced a function with an alias since >> the >> function is pointlessly there. It's duplicated code and the net result >> is >> just an alias. > > 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. > >>> Then, it's a bit too late to change 5.1.6, when 5.2.2 is on it's way, >>> so >>> surely >>> we would like to see the patches for 5.2.2 instead. >> >> This patch is against 5.1.6 and the 5.2 branch. The code in question has >> not been touched since before 5.1.0 was released. I tested it against >> both >> 5.2 branch and 5.1.6 prior to uploading it. > > Ok then. > >>> You can get its sources from CVS using PHP_5_2 branch: >>> cvs -d :pserver:[EMAIL PROTECTED]/repository co -r PHP_5_2 php-src >> >> You might want to update your instructions to people since the user is >> "cvsread" and not "anonymous". > > Oh, that's just my memory. > > -- > 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
