Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:28187
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 212.25.124.162 as permitted sender)
Message-ID: <45E5F83F.7080308@zend.com>
Date: Thu, 01 Mar 2007 00:46:39 +0300
User-Agent: Thunderbird 2.0b2 (X11/20070116)
MIME-Version: 1.0
To:  cardoe@gentoo.org
CC:  internals@lists.php.net
References: <40059.216.155.111.10.1172694790.squirrel@webmail.cardoe.com>       <45E5EA50.3070800@zend.com>    <33449.216.155.111.10.1172696679.squirrel@webmail.cardoe.com>    <45E5F067.3020709@zend.com> <58735.216.155.111.10.1172698524.squirrel@webmail.cardoe.com>
In-Reply-To: <58735.216.155.111.10.1172698524.squirrel@webmail.cardoe.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] LDAP functions implemented poorly
From: antony@zend.com (Antony Dovgal)

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?

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?

>> 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.

-- 
Wbr, 
Antony Dovgal