(against 7.1) Fix bug 72610 by deferring __wakeup: fixes unserialize read after free of property table

[TysonAndre]

Same as what used to be #2004 , but against php 7.1. Further changes will be made against #2004 to preserve the ABI.

This changes the ABI, but 7.1 is still in beta. See #2004 (comment)

[dshafik]

Seems @nikic already merged this into 7.1: #2070 (comment)

Please confirm and close if so :)

[TysonAndre]

Fixed merge conflicts. This PR is still applicable, but I didn't notice #2070 being created or merged.

The two XFAIL tests in ext/standard/tests/serializer are still failing (currently crashing).

[nikic]

The implementation currently will wake up all queued objects on completion of nested unserialize, including those not part of the nested unserialize. I assume this done, because the nested unserialize may contain references to those objects and a woken-up state may be subsequently assumed?

Independently of the exact set of objects that are woken up, any Serializable object can still be used to cause the same memory unsafety (without anything nefarious in the unserialize handler itself) using a sequence like ObjWithWakeup;ArrayObject;Ref;.

This could be avoided by delaying the wakeups until the very end (unserialize data destruction) -- however, this does change the call sequence. I'm wondering if it might still be preferable, as the behavior difference should be insignificant (just like none of the core Serializable handlers would care).

[TysonAndre]

The implementation currently will wake up all queued objects on completion of nested unserialize, including those not part of the nested unserialize. I assume this done, because the nested unserialize may contain references to those objects and a woken-up state may be subsequently assumed?

I assume by nested unserialize, you mean internal functions defining foo_ce->unserialize = foo_unserialize. Anything else? (Internal classes such as DateTime might also define __wakeup with the syntax PHP_METHOD(DateTime, __wakeup)

Nested unserializing behavior as currently written in this PR was an oversight... there's probably no catch-all way if internal classes define their own unserialize() function.

• Instead, I might be able to assume that internal classes don't define dynamic params in object props on __wakeup, and assume bug reports would be filed

static int php_var_unserialize_internal(UNSERIALIZE_PARAMETER);would need to be changed tostatic int php_var_unserialize_internal(UNSERIALIZE_PARAMETER, php_wakeup_data *data)`

using a sequence like ObjWithWakeup;ArrayObject;Ref;.

I haven't seen that bug report for ArrayObject, are you able to link that or example code?

[nikic]

By nested unserialize I mean Serializable::unserialize() calling unserialize(), which runs in the same unserialization context (shared unserialize data). That includes a number of internal classes, but can also be specified in userland. Generally, ::unserialize() is affected in the same way as __wakeup, but I don't think there is a good way to resolve that, as those calls cannot be delayed.

My concern is with the case where ::unserialize() does not do anything nefarious itself, but simply triggers wakeups in a nested unserialize (at least with the current patch). ArrayObject is an example of a class using Serializable. For an example of this problem, simply insert a new ArrayObject between the two objects in https://github.com/php/php-src/pull/2050/files#diff-135a455a26203243ea45bfccd93d0b5cR25

[krakjoe]

@nikic this has merge conflicts, also, I am unsure if this is the patch I'm told exists by Stas, or is that another one ?

[nikic]

The patch Stas mentioned is a different one, although based on the same general idea. As it should be landing soon, I'm closing this PR and #2004. I'll write back once the patch lands.

[weltling]

ACK, i'll be waiting for the other __wakeup patch as well, to include for the RC as recommmended by Stas.

Thanks.