# Fix for bug #63437

- Thread: https://externals.io/message/66611
- Original archive: https://news-web.php.net/php.internals/66611
- Started by Anatol Belski on 2013-03-14T13:14:15+00:00
- Messages: 3
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #66611 — Anatol Belski — 2013-03-14T13:14:15+00:00

URL: https://externals.io/message/66611#66611

> 
>
>
> > […]
>
> 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

---

## #66614 — Anatol Belski — 2013-03-14T15:32:36+00:00

URL: https://externals.io/message/66611#66614 · In reply to: #66611

> 
>
> > […]
>
> 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.
>
> > […]
>
> 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

---

## #66892 — Pierre Joye — 2013-04-02T16:03:40+00:00

URL: https://externals.io/message/66611#66892 · In reply to: #66614

hi,



> > […]
>
> 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