Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:37892
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: <483A9FAC.9040706@zend.com>
Date: Mon, 26 May 2008 15:31:56 +0400
User-Agent: Thunderbird 2.0.0.14 (X11/20080501)
MIME-Version: 1.0
To: Matt Wilmas <php_lists@realplain.com>
CC: internals@lists.php.net
References: <014b01c8a6db$57d53ba0$0201a8c0@pc1> <48181C02.3050705@zend.com> <00a701c8aaa9$e0fcd4c0$0201a8c0@pc1> <48184609.5000804@zend.com> <00cf01c8aab0$0be2fb50$0201a8c0@pc1> <48185599.1030308@zend.com> <00a901c8bf1a$b46e6dd0$0201a8c0@pc1>
In-Reply-To: <00a901c8bf1a$b46e6dd0$0201a8c0@pc1>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] [PATCH] More array filling optimizations
From: dmitry@zend.com (Dmitry Stogov)

Hi Matt,

Sorry, but I'm very busy in the following two weeks (have to travel a 
lot). I'll try not to forget about you patch, but it would be better if 
you remember me after June 7.

Thanks. Dmitry.

Matt Wilmas wrote:
> Hi Dmitry, all,
> 
> I looked into and fixed the -1 size and memory overflow error a couple weeks
> or so ago, and finally updated the patches now. :-)  I made the mistake of
> assuming the code that checks offset/length (in array_splice, etc.) set
> length correctly, but it can still be negative, which broke
> array_init_size...  Should be OK now there, and also in array_slice and
> array_chunk.
> 
> So if it works correctly now, it sounded like the patch could be used?
> (This doesn't have the compile/VM part, of course.)  I would be able to
> commit these things myself now, but wouldn't unless I was given the OK
> first! :-)
> 
> http://realplain.com/php/array_init_size.diff
> http://realplain.com/php/array_init_size_5_3.diff
> 
> 
> Thanks,
> Matt
> 
> 
> ----- Original Message -----
> From: "Dmitry Stogov"
> Sent: Wednesday, April 30, 2008
> 
>> Matt Wilmas wrote:
>>> Hi again Dmitry,
>>>
>>> Hmm, if a -1 size was passed, I thought it would just make a huge array
> size
>>> (bad, obviously), but the tests would still work.  (Or I guess it might
>>> exhaust memory_limit.)  Anyway, the code for checking the size (length
>>> variable) in array_splice() is copied from php_splice(), so maybe
> there's
>>> some other error if it's becoming -1...
>> The bug causes memory overflow error, but the size definitely shouldn't
>> be -1, as the same tests work fine before the patch.
>>
>>> You don't think it makes sense to check return_value_used like
>>> next/prev/end/reset do?  They would only create a 1 element array, but
>>> array_splice() could waste time filling a large array that isn't used.
> I
>>> think the function is used a lot to only remove/replace elements, not
> using
>>> the return_value:
>>> http://www.google.com/codesearch?q=lang%3Aphp+array_splice
>> Ah. I see. My mistake. Of course it makes sense.
>>
>> Thanks. Dmitry.
>>
>>> If there's a -1 size bug, the array_splice changes can be reverted and
>>> simply have:
>>>
>>> if (return_value_used) {
>>>     /* Initialize return value */
>>>     array_init(return_value);
>>>     rem_hash = &Z_ARRVAL_P(return_value);
>>> }
>>>
>>> - Matt
>>>
>>>
>>> ----- Original Message -----
>>> From: "Dmitry Stogov"
>>> Sent: Wednesday, April 30, 2008
>>>
>>>> For some reason "make test" with the patch reported several broken
>>>> array_splice() tests. Looking in gdb I saw that init_array() got -1 as
> a
>>>> size of new array.
>>>>
>>>> I don't think checks for return_value_used for array_splice() have a
> lot
>>>> of sense.
>>>>
>>>> Thanks. Dmitry.
>