Re: LDAP functions implemented poorly

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?

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.

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:anonymous@cvs.php.net/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