Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:28184
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain gentoo.org does not designate 64.111.100.15 as permitted sender)
Message-ID: <33449.216.155.111.10.1172696679.squirrel@webmail.cardoe.com>
In-Reply-To: <45E5EA50.3070800@zend.com>
References: <40059.216.155.111.10.1172694790.squirrel@webmail.cardoe.com>
    <45E5EA50.3070800@zend.com>
Date: Wed, 28 Feb 2007 16:04:39 -0500 (EST)
To: internals@lists.php.net
Reply-To: cardoe@gentoo.org
User-Agent: SquirrelMail/1.4.9a
MIME-Version: 1.0
Content-Type: text/plain;charset=iso-8859-1
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] LDAP functions implemented poorly
From: cardoe@gentoo.org ("Doug Goldstein")

Antony Dovgal wrote:
> Hello.
> Thanks for sending the patch here.
>
> On 02/28/2007 11:33 PM, Doug Goldstein wrote:
>> Referencing Bug #38819 & Bug #40671
>> http://bugs.php.net/bug.php?id=38819
>> http://bugs.php.net/bug.php?id=40671
>>
>> Essentially I looked through the above mentioned bug, the bugs opened
>> with OpenLDAP developers, and then reviewed ext/ldap/ldap.c and it
>> appears the API calls made by PHP are not necessarily the safest ways to
>> write the PHP wrapper functions. Based on tony2001@php.net's comment
>> that the LDAP module is unmaintained I went ahead and made some
>> changes.
>
>> The functions char **ldap_get_values(ld, entry, attr) and struct berval
>> **ldap_get_values_len(ld, entry, attr) are essentially inter-changeable.
>> The big difference being that the berval struct provides you with a char
>> * and the size_t of the data. Rather then just a char * that you then
>> have to strlen() which will result in problems if the returned data is
>> not NULL terminated data. PHP's internal functions make the mistake of
>> assuming all data will be string data (NULL terminated char *) data,
>> which is the cause of the crash in bug #38819.
>
> 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.

>
>> The patch attached removes all of those assumptions and uses
>> ldap_get_values_len() and uses the length provided back by the structure
>> to feed add_index_stringl() instead of using add_index_string() which
>> will call it's own strlen() on the provided data.
>>
>> This patch also removes ldap_get_values() as a PHP function and makes it
>> an alias of ldap_get_values_len() since there's no difference and the
>> same data can be returned, it's just a safer version.
>
> 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.

>
> 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.


> 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".
>
> --
> Wbr,
> Antony Dovgal
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>