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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
