From f151074db51c36280bdebfdbba14ce4ba07ccb62 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Mon, 26 Oct 2026 09:52:19 -0400 Subject: [PATCH] Fix GH-24674: Share the unserialize context only with Serializable php_var_unserialize_init() joined the context of any parse in progress, so user code that runs mid-parse and calls unserialize() resolved back-references into the outer value, including values being destroyed: a destructor run when an __unserialize() payload fails to parse or by the garbage collector, an error handler for a parser warning, an allowed_classes __toString(), a session_decode() overwrite. Only the unserialize handler of a C: payload is meant to share it. object_custom() now publishes the context for the duration of that call, a nested init shares it only while serialize_lock is not held, as serialize() does, and a parse hides it for its own duration. Fiber switches are blocked and a bailout restores the previous context while it is published. A nested init under serialize_lock, as in a C: payload unserialized from __wakeup() or __unserialize(), now keeps the allowed_classes and max_depth of the payload. Fixes GH-24674 --- NEWS | 2 + UPGRADING | 11 +++ UPGRADING.INTERNALS | 8 ++ ext/session/tests/gh24674.phpt | 32 ++++++++ ext/standard/basic_functions.h | 1 - ext/standard/tests/serialize/gh24674.phpt | 67 ++++++++++++++++ .../tests/serialize/gh24674_fiber.phpt | 42 ++++++++++ ext/standard/tests/serialize/gh24674_gc.phpt | 39 ++++++++++ .../tests/serialize/gh24674_wakeup.phpt | 45 +++++++++++ ext/standard/var.c | 7 +- ext/standard/var_unserializer.re | 78 ++++++++++++------- 11 files changed, 302 insertions(+), 30 deletions(-) create mode 100644 ext/session/tests/gh24674.phpt create mode 100644 ext/standard/tests/serialize/gh24674.phpt create mode 100644 ext/standard/tests/serialize/gh24674_fiber.phpt create mode 100644 ext/standard/tests/serialize/gh24674_gc.phpt create mode 100644 ext/standard/tests/serialize/gh24674_wakeup.phpt diff --git a/NEWS b/NEWS index cd9ddd3994cc..71a2ccd64fd7 100644 --- a/NEWS +++ b/NEWS @@ -152,6 +152,8 @@ PHP NEWS lookup, and an unchecked CreateFileA() failure. (Ilia Alshanetsky) . Fix persistent stream context lifetime during shutdown (Levi Morrison) . Fixed bug GH-23780 (unserialize('') fails silently). (timwolla) + . Fixed bug GH-24674 (unserialize() called while another value is being + unserialized reads that value's back-references). (Ilia Alshanetsky) . Fixed stream_select() aborting the whole call once any watched stream's descriptor exceeded FD_SETSIZE; it no longer imposes that limit, so watching many descriptors no longer requires rebuilding with --enable-fd-setsize. diff --git a/UPGRADING b/UPGRADING index 49d1ebc8258c..0f5b1b9c6758 100644 --- a/UPGRADING +++ b/UPGRADING @@ -55,6 +55,17 @@ PHP 8.7 UPGRADE NOTES . The number of filters that can be chained in a php://filter URL is limited to 16 by default. Set the stream context option max_filter_count to change this. + . unserialize() called from a destructor, an error handler or another + callback that runs while a value is being unserialized no longer resolves + R: and r: back-references into the outer value. Only the + Serializable::unserialize() call made for a C: payload, and code it calls, + shares them. + . A C: payload unserialized from __wakeup() or __unserialize() now honours + the allowed_classes and max_depth options of that unserialize() call, and + unserialize() calls made while it is processed inherit them. + . Starting, suspending or resuming a Fiber inside the + Serializable::unserialize() call made by unserialize() for a C: payload + now throws a FiberError. ======================================== 2. New Features diff --git a/UPGRADING.INTERNALS b/UPGRADING.INTERNALS index 4c5d74b49e9c..b8e2774eec47 100644 --- a/UPGRADING.INTERNALS +++ b/UPGRADING.INTERNALS @@ -77,6 +77,14 @@ PHP 8.7 INTERNALS UPGRADE NOTES - Standard: . php_explode() now takes a HashTable* instead of a zval* to store the resulting parts. It already expected the zval* to be IS_ARRAY. + . BG(unserialize).level has been removed. BG(unserialize).data is now set + only while unserialize() calls the unserialize handler of a C: payload, + and is NULL otherwise; code that writes it must save and restore it. The + first PHP_VAR_UNSERIALIZE_INIT() made while it is set shares that + context until the matching PHP_VAR_UNSERIALIZE_DESTROY() if + BG(serialize_lock) is zero, as php_var_serialize_init() does. Otherwise + it creates its own context, which takes allowed_classes, max_depth and + incomplete_classes_only from the shared one. ======================== 4. OpCode changes diff --git a/ext/session/tests/gh24674.phpt b/ext/session/tests/gh24674.phpt new file mode 100644 index 000000000000..baf90f779b2b --- /dev/null +++ b/ext/session/tests/gh24674.phpt @@ -0,0 +1,32 @@ +--TEST-- +GH-24674 (Destructor run by session_decode() gets its own unserialize() context) +--EXTENSIONS-- +session +--INI-- +session.use_cookies=0 +session.cache_limiter= +session.serialize_handler=php +session.save_handler=files +--FILE-- + +--EXPECT-- +destructor: bool(false) +array(2) { + ["a"]=> + int(5) + ["b"]=> + string(6) "secret" +} diff --git a/ext/standard/basic_functions.h b/ext/standard/basic_functions.h index 10dfd584d704..b7f30953cb26 100644 --- a/ext/standard/basic_functions.h +++ b/ext/standard/basic_functions.h @@ -88,7 +88,6 @@ typedef struct _php_basic_globals { } serialize; struct { struct php_unserialize_data *data; - unsigned level; } unserialize; /* url_scanner_ex.re */ diff --git a/ext/standard/tests/serialize/gh24674.phpt b/ext/standard/tests/serialize/gh24674.phpt new file mode 100644 index 000000000000..4e14b4501f59 --- /dev/null +++ b/ext/standard/tests/serialize/gh24674.phpt @@ -0,0 +1,67 @@ +--TEST-- +GH-24674 (Nested unserialize() shares the context of a parse in progress) +--FILE-- + [new N]])); + +class S implements Serializable { + public $o; + public function serialize() { return ''; } + public function unserialize($data) { $this->o = unserialize($data); } + public function __serialize(): array { return []; } + public function __unserialize(array $data): void {} +} + +$r = unserialize('a:2:{i:0;O:8:"stdClass":0:{}i:1;C:1:"S":4:{r:2;}}'); +echo "Serializable back-reference: "; +var_dump($r[1]->o === $r[0]); +?> +--EXPECT-- +destructor: bool(false) +bool(false) +Creation of dynamic property P::$dyn is deprecated: bool(false) +bool(true) +unserialize(): Extra data starting at offset 29 of 33 bytes: bool(false) +array(1) { + [0]=> + object(stdClass)#2 (0) { + } +} +__toString: array(1) { + [0]=> + string(6) "secret" +} +bool(false) +Serializable back-reference: bool(true) diff --git a/ext/standard/tests/serialize/gh24674_fiber.phpt b/ext/standard/tests/serialize/gh24674_fiber.phpt new file mode 100644 index 000000000000..a092b0689581 --- /dev/null +++ b/ext/standard/tests/serialize/gh24674_fiber.phpt @@ -0,0 +1,42 @@ +--TEST-- +GH-24674 (Fiber switches are not allowed inside Serializable::unserialize()) +--FILE-- +start(); echo "start ok\n"; } + catch (FiberError $e) { echo "start: ", $e::class, ": ", $e->getMessage(), "\n"; } + } + public function __serialize(): array { return []; } + public function __unserialize(array $d): void {} +} +$g = new Fiber(function () { Fiber::suspend(); }); +$g->start(); +class R implements Serializable { + public function serialize() { return ''; } + public function unserialize($d) { + try { $GLOBALS['g']->resume(); echo "resume ok\n"; } + catch (FiberError $e) { echo "resume: ", $e::class, ": ", $e->getMessage(), "\n"; } + } + public function __serialize(): array { return []; } + public function __unserialize(array $d): void {} +} +unserialize('C:1:"S":0:{}'); +unserialize('C:1:"R":0:{}'); +$h = new Fiber(function () { unserialize('C:1:"T":0:{}'); }); +class T implements Serializable { + public function serialize() { return ''; } + public function unserialize($d) { + try { Fiber::suspend(); echo "suspend ok\n"; } + catch (FiberError $e) { echo "suspend: ", $e::class, ": ", $e->getMessage(), "\n"; } + } + public function __serialize(): array { return []; } + public function __unserialize(array $d): void {} +} +$h->start(); +?> +--EXPECT-- +start: FiberError: Cannot switch fibers in current execution context +resume: FiberError: Cannot switch fibers in current execution context +suspend: FiberError: Cannot switch fibers in current execution context diff --git a/ext/standard/tests/serialize/gh24674_gc.phpt b/ext/standard/tests/serialize/gh24674_gc.phpt new file mode 100644 index 000000000000..e9faa5df8e5d --- /dev/null +++ b/ext/standard/tests/serialize/gh24674_gc.phpt @@ -0,0 +1,39 @@ +--TEST-- +GH-24674 (GC destructor running during unserialize() gets its own context) +--FILE-- +self = $g; +} +unset($g); + +var_dump(@unserialize($payload)); +?> +--EXPECT-- +destructor: bool(false) +bool(false) diff --git a/ext/standard/tests/serialize/gh24674_wakeup.phpt b/ext/standard/tests/serialize/gh24674_wakeup.phpt new file mode 100644 index 000000000000..29e098ea18af --- /dev/null +++ b/ext/standard/tests/serialize/gh24674_wakeup.phpt @@ -0,0 +1,45 @@ +--TEST-- +GH-24674 (C: payload unserialized from __wakeup() or __unserialize()) +--FILE-- +data); } + public function unserialize($data) { $this->data = unserialize($data); } +} + +function check($where) { + $p = 'x:i:0;a:1:{i:0;O:1:"A":0:{}};m:a:0:{}'; + $r = unserialize('C:11:"ArrayObject":' . strlen($p) . ':{' . $p . '}', ['allowed_classes' => ['ArrayObject']]); + echo "$where allowed_classes: ", get_class($r[0]), "\n"; + + $o = new stdClass; + $s = new S; + $s->data = [$o, $o]; + $r = unserialize(serialize($s)); + echo "$where round trip: "; + var_dump($r->data[0] === $r->data[1]); +} + +class W { + public function __wakeup() { check('__wakeup'); } +} + +class U { + public function __unserialize(array $data): void { check('__unserialize'); } +} + +unserialize('O:1:"W":0:{}'); +unserialize('O:1:"U":0:{}'); +check('top level'); +?> +--EXPECTF-- +Deprecated: %s Serializable interface, which is deprecated. %s +__wakeup allowed_classes: __PHP_Incomplete_Class +__wakeup round trip: bool(true) +__unserialize allowed_classes: __PHP_Incomplete_Class +__unserialize round trip: bool(true) +top level allowed_classes: __PHP_Incomplete_Class +top level round trip: bool(true) diff --git a/ext/standard/var.c b/ext/standard/var.c index 689594ef240a..5dfd70794f11 100644 --- a/ext/standard/var.c +++ b/ext/standard/var.c @@ -1473,6 +1473,7 @@ PHPAPI void php_unserialize_with_options(zval *return_value, const char *buf, co zval *retval; HashTable *class_hash = NULL, *prev_class_hash; zend_long prev_max_depth, prev_cur_depth; + bool nested = BG(unserialize).data != NULL; p = (const unsigned char*) buf; PHP_VAR_UNSERIALIZE_INIT(var_hash); @@ -1539,7 +1540,7 @@ PHPAPI void php_unserialize_with_options(zval *return_value, const char *buf, co } } - if (BG(unserialize).level > 1) { + if (nested) { retval = var_tmp_var(&var_hash); } else { retval = return_value; @@ -1549,7 +1550,7 @@ PHPAPI void php_unserialize_with_options(zval *return_value, const char *buf, co php_error_docref(NULL, E_WARNING, "Error at offset " ZEND_LONG_FMT " of %zd bytes", (zend_long)((char*)p - buf), buf_len); } - if (BG(unserialize).level <= 1) { + if (!nested) { zval_ptr_dtor(return_value); } RETVAL_FALSE; @@ -1560,7 +1561,7 @@ PHPAPI void php_unserialize_with_options(zval *return_value, const char *buf, co (zend_long)((char*)p - buf), buf_len); } } - if (BG(unserialize).level > 1) { + if (nested) { ZVAL_COPY(return_value, retval); } else if (Z_REFCOUNTED_P(return_value)) { zend_refcounted *ref = Z_COUNTED_P(return_value); diff --git a/ext/standard/var_unserializer.re b/ext/standard/var_unserializer.re index 51c3293c63e2..91344e3a50ca 100644 --- a/ext/standard/var_unserializer.re +++ b/ext/standard/var_unserializer.re @@ -18,6 +18,7 @@ #include "zend_portability.h" #include "zend_exceptions.h" #include "zend_objects.h" +#include "zend_fibers.h" /* {{{ reference-handling for unserializer: var_* */ #define VAR_ENTRIES_MAX 1018 /* 1024 - offsetof(php_unserialize_data, entries) / sizeof(void*) */ @@ -58,44 +59,48 @@ struct php_unserialize_data { * __unserialize(). O:, C: and E: values all serialize back to the same * bytes. */ bool incomplete_classes_only; + uint32_t nesting; var_entries entries; }; PHPAPI php_unserialize_data_t php_var_unserialize_init(void) { + php_unserialize_data_t published = BG(unserialize).data; php_unserialize_data_t d; - /* fprintf(stderr, "UNSERIALIZE_INIT == lock: %u, level: %u\n", BG(serialize_lock), BG(unserialize).level); */ - if (BG(serialize_lock) || !BG(unserialize).level) { - d = emalloc(sizeof(struct php_unserialize_data)); - d->last = &d->entries; - d->first_dtor = d->last_dtor = NULL; + + if (published && !BG(serialize_lock)) { + BG(unserialize).data = NULL; + published->nesting++; + return published; + } + + d = emalloc(sizeof(struct php_unserialize_data)); + d->last = &d->entries; + d->first_dtor = d->last_dtor = NULL; + d->ref_props = NULL; + d->cur_depth = 0; + if (published) { + d->allowed_classes = published->allowed_classes; + d->max_depth = published->max_depth; + d->incomplete_classes_only = published->incomplete_classes_only; + } else { d->allowed_classes = NULL; - d->ref_props = NULL; - d->cur_depth = 0; d->max_depth = BG(unserialize_max_depth); d->incomplete_classes_only = false; - d->entries.used_slots = 0; - d->entries.next = NULL; - if (!BG(serialize_lock)) { - BG(unserialize).data = d; - BG(unserialize).level = 1; - } - } else { - d = BG(unserialize).data; - ++BG(unserialize).level; } + d->nesting = 0; + d->entries.used_slots = 0; + d->entries.next = NULL; return d; } PHPAPI void php_var_unserialize_destroy(php_unserialize_data_t d) { - /* fprintf(stderr, "UNSERIALIZE_DESTROY == lock: %u, level: %u\n", BG(serialize_lock), BG(unserialize).level); */ - if (BG(serialize_lock)) { - var_destroy(&d); - efree(d); - } else if (!--BG(unserialize).level) { - BG(unserialize).data = NULL; - var_destroy(&d); - efree(d); + if (d->nesting) { + d->nesting--; + BG(unserialize).data = d; + return; } + var_destroy(&d); + efree(d); } PHPAPI HashTable *php_var_unserialize_get_allowed_classes(php_unserialize_data_t d) { @@ -825,8 +830,26 @@ static inline int object_custom(UNSERIALIZE_PARAMETER, zend_class_entry *ce, boo return 0; } object_init_ex(rval, ce); - } else if (ce->unserialize(rval, ce, (const unsigned char*)*p, datalen, (zend_unserialize_data *)var_hash) != SUCCESS) { - return 0; + } else { + php_unserialize_data_t prev = BG(unserialize).data; + zend_result result = FAILURE; + bool bailout = false; + + BG(unserialize).data = *var_hash; + zend_fiber_switch_block(); + zend_try { + result = ce->unserialize(rval, ce, (const unsigned char*)*p, datalen, (zend_unserialize_data *)var_hash); + } zend_catch { + bailout = true; + } zend_end_try(); + zend_fiber_switch_unblock(); + BG(unserialize).data = prev; + if (bailout) { + zend_bailout(); + } + if (result != SUCCESS) { + return 0; + } } if (ZEND_HAS_OOM()) { @@ -912,9 +935,12 @@ PHPAPI int php_var_unserialize(UNSERIALIZE_PARAMETER) { var_entries *orig_var_entries = (*var_hash)->last; zend_long orig_used_slots = orig_var_entries ? orig_var_entries->used_slots : 0; + php_unserialize_data_t prev = BG(unserialize).data; int result; + BG(unserialize).data = NULL; result = php_var_unserialize_internal(UNSERIALIZE_PASSTHRU); + BG(unserialize).data = prev; if (!result) { /* If the unserialization failed, mark all elements that have been added to var_hash