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=latestSerializing 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
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=latestSerializing 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 DateIntervalobject (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 asunserialize()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
hi,
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 asunserialize()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