Re: [PHP-DEV] Fix for bug #63437
hi, On Thu, Mar 14, 2013 at 4:32 PM, Anatol Belski wrote: >> 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 Tests pass, crashes are gone and the patch meets the requirements Derick asked earlier. If there is no further objection until next Monday I will ask Anatolyi to apply it and finally fix one of the only remaining known crash in 5.3/4, and indeed 5.5 and master (but there are other there :). Cheers, -- Pierre @pierrejoye -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fix for bug #63437
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 +, 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
Re: [PHP-DEV] Fix for bug #63437
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 +, 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?
Regards
Anatol
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fix for bug #63437
On Wed, 13 Mar 2013, Anatol Belski wrote:
> On Mon, 2013-03-11 at 11:42 +, 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_var3.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.
And secondly, where are the test cases?
cheers,
Derick
--
http://derickrethans.nl | http://xdebug.org
Like Xdebug? Consider a donation: http://xdebug.org/donate.php
twitter: @derickr and @xdebug
Posted with an email client that doesn't mangle email: alpine
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fix for bug #63437
On Mon, 2013-03-11 at 11:42 +, Derick Rethans wrote: > Please, no top posting! > > On Mon, 11 Mar 2013, Anatol Belski wrote: > > On Sun, March 10, 2013 23:11, Derick Rethans wrote: > > > On Sat, 9 Mar 2013, Anatol Belski wrote: > > > > > >> On Sat, 2013-03-09 at 21:57 +0100, Gustavo Lopes wrote: > > >> > > >>> I would agree in principle, but, as I explained before, there is a > > >>> problem. The DatePeriod class has 64-bit integers in its internal > > >>> structure. The PHP integer type cannot (in general) represent that > > >>> data. So the general method of getting the object data via > > >>> get_properties and serializing (and then using __set_state to > > >>> convert the array back) does not work unless you represent those > > >>> 64-bit integers with some non-integer type and do the conversions. > > >> > > >> So base64 seems to be only the doubtful point. Thriving to fix > > >> that, what if we could bring it in dependency of libgmp to > > >> serialize and read as strings (and maybe disable serialization > > >> otherwise)? > > > > > > Why do you need libgmp for that‽ > > > libgmp was just the first shot as it has functions to convert from > > arbitrary binary data to string and vice versa, mpz_import and > > mpz_export. That's what should work fine across platforms. Looking at > > the type definitions here > > http://lxr.php.net/xref/PHP_5_5/ext/date/lib/timelib_structs.h#70 i > > wouldn't exclude possible platform issues. Implementing that manually > > is a tricky job, could be done probably with more homework :) > > > > 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_var3.patch&revision=latest Serializing 64 bit integers as strings. Regards Anatol > > cheers, > Derick > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fix for bug #63437
Please, no top posting! On Mon, 11 Mar 2013, Anatol Belski wrote: > On Sun, March 10, 2013 23:11, Derick Rethans wrote: > > On Sat, 9 Mar 2013, Anatol Belski wrote: > > > >> On Sat, 2013-03-09 at 21:57 +0100, Gustavo Lopes wrote: > >> > >>> I would agree in principle, but, as I explained before, there is a > >>> problem. The DatePeriod class has 64-bit integers in its internal > >>> structure. The PHP integer type cannot (in general) represent that > >>> data. So the general method of getting the object data via > >>> get_properties and serializing (and then using __set_state to > >>> convert the array back) does not work unless you represent those > >>> 64-bit integers with some non-integer type and do the conversions. > >> > >> So base64 seems to be only the doubtful point. Thriving to fix > >> that, what if we could bring it in dependency of libgmp to > >> serialize and read as strings (and maybe disable serialization > >> otherwise)? > > > > Why do you need libgmp for that‽ > libgmp was just the first shot as it has functions to convert from > arbitrary binary data to string and vice versa, mpz_import and > mpz_export. That's what should work fine across platforms. Looking at > the type definitions here > http://lxr.php.net/xref/PHP_5_5/ext/date/lib/timelib_structs.h#70 i > wouldn't exclude possible platform issues. Implementing that manually > is a tricky job, could be done probably with more homework :) > > What is the way you had in the mind to achieve the string<->integer > conversions? atoll() (or atoq()). cheers, Derick -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fix for bug #63437
libgmp was just the first shot as it has functions to convert from arbitrary binary data to string and vice versa, mpz_import and mpz_export. That's what should work fine across platforms. Looking at the type definitions here http://lxr.php.net/xref/PHP_5_5/ext/date/lib/timelib_structs.h#70 i wouldn't exclude possible platform issues. Implementing that manually is a tricky job, could be done probably with more homework :) What is the way you had in the mind to achieve the string<->integer conversions? Regards Anatol On Sun, March 10, 2013 23:11, Derick Rethans wrote: > On Sat, 9 Mar 2013, Anatol Belski wrote: > > >> On Sat, 2013-03-09 at 21:57 +0100, Gustavo Lopes wrote: >> >>> On Sat, 09 Mar 2013 21:36:41 +0100, Derick Rethans >>> wrote: >>> >>> On Tue, 5 Mar 2013, Anatol Belski wrote: > I've reworked the patch from > http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff > (mentioned by tony2001) for bug #63437, that seems to fix the > issue. That patch was ported back to 5.3 and adapted to the current > 5.4+. Both variants are posted to the ticket. > Serializing this as a base64 encoded variant of some binary data is not a good thing. If you want to serialize, it needs to output the same thigns that allow users to create the period or interval. >>> >>> I would agree in principle, but, as I explained before, there is a >>> problem. The DatePeriod class has 64-bit integers in its internal >>> structure. The PHP integer type cannot (in general) represent that >>> data. So the general method of getting the object data via >>> get_properties and serializing (and then using __set_state to convert >>> the array back) does not work unless you represent those 64-bit >>> integers with some non-integer type and do the conversions. >> >> So base64 seems to be only the doubtful point. Thriving to fix that, >> what if we could bring it in dependency of libgmp to serialize and read >> as strings (and maybe disable serialization otherwise)? > > Why do you need libgmp for that‽ > > > cheers, Derick -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fix for bug #63437
On Sat, 9 Mar 2013, Anatol Belski wrote: > On Sat, 2013-03-09 at 21:57 +0100, Gustavo Lopes wrote: > > On Sat, 09 Mar 2013 21:36:41 +0100, Derick Rethans wrote: > > > > > On Tue, 5 Mar 2013, Anatol Belski wrote: > > > > > >> I've reworked the patch from > > >> http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff > > >> (mentioned by tony2001) for bug #63437, that seems to fix the > > >> issue. That patch was ported back to 5.3 and adapted to the > > >> current 5.4+. Both variants are posted to the ticket. > > > > > > Serializing this as a base64 encoded variant of some binary data > > > is not a good thing. If you want to serialize, it needs to output > > > the same thigns that allow users to create the period or interval. > > > > I would agree in principle, but, as I explained before, there is a > > problem. The DatePeriod class has 64-bit integers in its internal > > structure. The PHP integer type cannot (in general) represent that > > data. So the general method of getting the object data via > > get_properties and serializing (and then using __set_state to > > convert the array back) does not work unless you represent those > > 64-bit integers with some non-integer type and do the conversions. > > So base64 seems to be only the doubtful point. Thriving to fix that, > what if we could bring it in dependency of libgmp to serialize and > read as strings (and maybe disable serialization otherwise)? Why do you need libgmp for that‽ cheers, Derick -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fix for bug #63437
On Sat, 9 Mar 2013, Gustavo Lopes wrote: > On Sat, 09 Mar 2013 21:36:41 +0100, Derick Rethans wrote: > > > On Tue, 5 Mar 2013, Anatol Belski wrote: > > > > > I've reworked the patch from > > > http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff > > > (mentioned by tony2001) for bug #63437, that seems to fix the > > > issue. That patch was ported back to 5.3 and adapted to the > > > current 5.4+. Both variants are posted to the ticket. > > > > Serializing this as a base64 encoded variant of some binary data is > > not a good thing. If you want to serialize, it needs to output the > > same thigns that allow users to create the period or interval. > > I would agree in principle, but, as I explained before, there is a > problem. The DatePeriod class has 64-bit integers in its internal > structure. The PHP integer type cannot (in general) represent that > data. So the general method of getting the object data via > get_properties and serializing (and then using __set_state to convert > the array back) does not work unless you represent those 64-bit > integers with some non-integer type and do the conversions. Then store them as strings or another format that makes it possible to get a 64bit timestamp back. Of course, I don't think it makes too much sense to serialize a Period as it's more of a transient iterator wrapper. For an interval, you'd want to serialise the original input vectors as well... not an easy task (unless you store the original). cheers, Derick -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fix for bug #63437
Hi, On Sat, 2013-03-09 at 21:57 +0100, Gustavo Lopes wrote: > On Sat, 09 Mar 2013 21:36:41 +0100, Derick Rethans wrote: > > > On Tue, 5 Mar 2013, Anatol Belski wrote: > > > >> I've reworked the patch from > >> http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff > >> (mentioned by tony2001) for bug #63437, that seems to fix the issue. > >> That patch was ported back to 5.3 and adapted to the current 5.4+. > >> Both variants are posted to the ticket. > > > > Serializing this as a base64 encoded variant of some binary data is not > > a good thing. If you want to serialize, it needs to output the same > > thigns that allow users to create the period or interval. > > I would agree in principle, but, as I explained before, there is a > problem. The DatePeriod class has 64-bit integers in its internal > structure. The PHP integer type cannot (in general) represent that data. > So the general method of getting the object data via get_properties and > serializing (and then using __set_state to convert the array back) does > not work unless you represent those 64-bit integers with some non-integer > type and do the conversions. So base64 seems to be only the doubtful point. Thriving to fix that, what if we could bring it in dependency of libgmp to serialize and read as strings (and maybe disable serialization otherwise)? > > > I also don't think this would work for Big Endian vs Little Endian > > either. > > It does; the integers are converted to network order before being stored, > so you can share the serialized data between machines with different > endianness. > > > -- > Gustavo Lopes > -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fix for bug #63437
On Sat, 09 Mar 2013 21:36:41 +0100, Derick Rethans wrote: On Tue, 5 Mar 2013, Anatol Belski wrote: I've reworked the patch from http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff (mentioned by tony2001) for bug #63437, that seems to fix the issue. That patch was ported back to 5.3 and adapted to the current 5.4+. Both variants are posted to the ticket. Serializing this as a base64 encoded variant of some binary data is not a good thing. If you want to serialize, it needs to output the same thigns that allow users to create the period or interval. I would agree in principle, but, as I explained before, there is a problem. The DatePeriod class has 64-bit integers in its internal structure. The PHP integer type cannot (in general) represent that data. So the general method of getting the object data via get_properties and serializing (and then using __set_state to convert the array back) does not work unless you represent those 64-bit integers with some non-integer type and do the conversions. I also don't think this would work for Big Endian vs Little Endian either. It does; the integers are converted to network order before being stored, so you can share the serialized data between machines with different endianness. -- Gustavo Lopes -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fix for bug #63437
On Tue, 5 Mar 2013, Anatol Belski wrote: > I've reworked the patch from > http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff > (mentioned by tony2001) for bug #63437, that seems to fix the issue. > That patch was ported back to 5.3 and adapted to the current 5.4+. > Both variants are posted to the ticket. Serializing this as a base64 encoded variant of some binary data is not a good thing. If you want to serialize, it needs to output the same thigns that allow users to create the period or interval. I also don't think this would work for Big Endian vs Little Endian either. cheers, Derick -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Fix for bug #63437
Sorry, the correct one is bug #53437 ... On Tue, March 5, 2013 12:42, Anatol Belski wrote: > Hi, > > > I've reworked the patch from > http://nebm.ist.utl.pt/~glopes/misc/date_period_interval_ser.diff > (mentioned by tony2001) for bug #63437, that seems to fix the issue. That > patch was ported back to 5.3 and adapted to the current 5.4+. Both variants > are posted to the ticket. > > Also the test for bug #52113 delivers more plausible results and should > be fixed when the patch is applied. > > Regards > > > Anatol > > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > > > -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
