Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:66614
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain php.net does not designate 85.214.73.107 as permitted sender)
Message-ID: <eece2e4b8e71c073ae35b8d57b109c54.squirrel@webmail.klapt.com>
In-Reply-To: <0728f531a623e3f9b7f0897a9c6fabb7.squirrel@webmail.klapt.com>
References: <0728f531a623e3f9b7f0897a9c6fabb7.squirrel@webmail.klapt.com>
Date: Thu, 14 Mar 2013 16:32:36 +0100
To: "Derick Rethans" <derick@php.net>
Cc: "Gustavo Lopes" <glopes@nebm.ist.utl.pt>,
 "PHP Developers Mailing List" <internals@lists.php.net>
Reply-To: ab@php.net
User-Agent: SquirrelMail/1.5.2 [SVN]
MIME-Version: 1.0
Content-Type: text/plain;charset=UTF-8
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] Fix for bug #63437
From: ab@php.net ("Anatol Belski")

On Thu, March 14, 2013 14:14, Anatol Belski wrote:
> On Thu, March 14, 2013 12:42, Derick Rethans wrote:
>
>> On Wed, 13 Mar 2013, Anatol Belski wrote:
>>
>>
>>
>>> On Mon, 2013-03-11 at 11:42 +0000, Derick Rethans wrote:
>>>
>>>
>>>>
>>>> On Mon, 11 Mar 2013, Anatol Belski wrote:
>>>>
>>>>
>>>>>
>>>>> What is the way you had in the mind to achieve the
>>>>> string<->integer conversions?
>>>>
>>>> atoll() (or atoq()).
>>>
>>> Please take a look at the reworked patch in #53437
>>>
>>>
>>>
>>> https://bugs.php.net/patch-display.php?bug_id=53437&patch=date_patch_
>>> va r3.patch&revision=latest
>>>
>>> Serializing 64 bit integers as strings.
>>>
>>>
>>
>> That bit looks ok, I have a few comments though:
>>
>>
>>
>> +	PHP_DATE_INTERVAL_READ_PROPERTY("special_type", special.type,
>> unsigned int); +	if ((*intobj)->diff->special.amount > 12 ||
>> (*intobj)->diff->special.amount < -12) {
>> +		php_error(E_ERROR, "Invalid serialization data for DateInterval
>> object (special_amount)");
>> +	}
>>
>>
>>
>> Checking for the values here, means that the library (timelib) isn't
>> in control of the values that are allowed anymore. I wonder whether the
>> deserialisation should bother checking some of those special/extra
>> types/values. The only important thing is to not have things crash
>> really.
>>
>
> I think that checks can be safely removed. I just reintegrated them from
> the previous patch. It wouldn't crash with those removed. I'll recheck it
> with the timelib and remove them.
>
>>
>> And secondly, where are the test cases?
>>
>>
>
> Just wanted to hear your OK in general. The test is primarily the
> iteratior snippet from the ticket plus a couple of bad serialization
> strings. As there are about 10 tests having to be fixed after this change
>  (mostly because of the changed var_dump output), i'll do them all
> together while commiting.
>
>
> One doubt I have yet after investigating on #62852 is that issuing
> php_error isn't recoverable, it might be much better to throw exception in
>  __wakeup(), just like __construct() does. This question crosses both
> #62852  and #53437. That would work for 5.5 as unserialize() cares about
> exceptions in __wakeup(), in 5.4 and less php_error seems to be a better
> way. What do you think?
>

I've reuploaded the patch, removed that extra checks, fixed the relevant
tests, removed XFAIL from bug53437.phpt and added three more.

https://bugs.php.net/patch-display.php?bug_id=53437&patch=date_patch_var4.patch&revision=latest

Regards

Anatol