From fb6838770c5c8b2ba05fcac737ee93fbe5087466 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 7 Oct 2023 23:46:29 +0200 Subject: [PATCH 1/2] Fix GH-12380: JIT+private array property access inside closure accesses private property in child class For private fields, the scope has to be taken into account, otherwise the property info may come from the wrong ce. Closes GH-12381. --- NEWS | 2 + ext/opcache/jit/zend_jit.c | 6 ++- ext/opcache/tests/jit/gh12380.phpt | 62 ++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 ext/opcache/tests/jit/gh12380.phpt diff --git a/NEWS b/NEWS index 5bdb6873238f4..c7fc425ec1c2f 100644 --- a/NEWS +++ b/NEWS @@ -42,6 +42,8 @@ PHP NEWS - Opcache: . Fixed opcache_invalidate() on deleted file. (mikhainin) + . Fixed bug GH-12380 (JIT+private array property access inside closure + accesses private property in child class). (nielsdos) - PCRE: . Fixed bug GH-11956 (Backport upstream fix, PCRE regular expressions with diff --git a/ext/opcache/jit/zend_jit.c b/ext/opcache/jit/zend_jit.c index 8c6934dfb6358..d969f744e6b4b 100644 --- a/ext/opcache/jit/zend_jit.c +++ b/ext/opcache/jit/zend_jit.c @@ -650,7 +650,11 @@ static zend_property_info* zend_get_known_property_info(const zend_op_array *op_ return info; } else if (on_this) { if (ce == info->ce) { - return info; + if (ce == op_array->scope) { + return info; + } else { + return NULL; + } } else if ((info->flags & ZEND_ACC_PROTECTED) && instanceof_function_slow(ce, info->ce)) { return info; diff --git a/ext/opcache/tests/jit/gh12380.phpt b/ext/opcache/tests/jit/gh12380.phpt new file mode 100644 index 0000000000000..75a0a9cc41824 --- /dev/null +++ b/ext/opcache/tests/jit/gh12380.phpt @@ -0,0 +1,62 @@ +--TEST-- +GH-12380: JIT+private array property access inside closure accesses private property in child class +--INI-- +opcache.enable=1 +opcache.enable_cli=1 +opcache.file_update_protection=0 +opcache.jit_buffer_size=1M +opcache.protect_memory=1 +opcache.jit=tracing +opcache.jit_hot_loop=1 +opcache.jit_hot_func=1 +opcache.jit_hot_return=1 +opcache.jit_hot_side_exit=1 +--EXTENSIONS-- +opcache +--FILE-- +v); + (function (): void { + var_dump($this->v); + })(); + } +} + +final class b extends a { + private int $v = 0; +} +$a = new b; + +for ($i = 0; $i < 10; $i++) { + $a->test(); +} + +?> +--EXPECT-- +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) +int(1) From 24e5e4ec0d254f53eaf9e7e0994b7c6b7a383eaa Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 9 Oct 2023 00:15:54 +0200 Subject: [PATCH 2/2] Fix GH-8996: DOMNode serialization on PHP ^8.1 PHP 8.1 introduced a seemingly unintentional BC break in ca94d55a19 by blocking the (un)serialization of DOM objects. This was done because the serialization never really worked and just resulted in an empty object, which upon unserialization just resulted in an object that you can't use. Users can however implement their own serialization methods, but the commit made that impossible as the ACC flag gets passed down to the child class. An approach was tried in #10307 with a new ACC flag to selectively allow serialization with subclasses if they implement the right methods. However, that was found to be too ad hoc. Instead, let's abuse how the __sleep and __wakeup methods work to throw the exception instead. If the child class implements the __serialize / __unserialize method, then the throwing methods won't be called. Similarly, if the child class implements __sleep and __wakeup, then they're overridden and it doesn't matter that they throw. For the user, this PR has the exact same behaviour for (sub)classes that don't implement the serialization methods: an exception will be thrown. For code that previously implemented subclasses with these methods, this approach will make that code work again. This approach should be both BC preserving and unbreak user's code. Closes GH-12388. For the test: Co-authored-by: wazelin --- NEWS | 1 + ext/dom/node.c | 21 +++++ ext/dom/php_dom.stub.php | 12 ++- ext/dom/php_dom_arginfo.h | 19 +++- ext/dom/tests/gh8996.phpt | 120 ++++++++++++++++++++++++++ ext/dom/tests/not_serializable.phpt | 6 +- ext/dom/tests/not_unserializable.phpt | 29 +++++++ 7 files changed, 200 insertions(+), 8 deletions(-) create mode 100644 ext/dom/tests/gh8996.phpt create mode 100644 ext/dom/tests/not_unserializable.phpt diff --git a/NEWS b/NEWS index c7fc425ec1c2f..694493cca265a 100644 --- a/NEWS +++ b/NEWS @@ -19,6 +19,7 @@ PHP NEWS - DOM: . Restore old namespace reconciliation behaviour. (nielsdos) + . Fixed bug GH-8996 (DOMNode serialization on PHP ^8.1). (nielsdos) - Fileinfo: . Fixed bug GH-11891 (fileinfo returns text/xml for some svg files). (usarise) diff --git a/ext/dom/node.c b/ext/dom/node.c index bcf4ee487d38d..7a1e879575c80 100644 --- a/ext/dom/node.c +++ b/ext/dom/node.c @@ -1786,4 +1786,25 @@ PHP_METHOD(DOMNode, getLineNo) } /* }}} */ +/** + * We want to block the serialization and unserialization of DOM classes. + * However, using @not-serializable makes the child classes also not serializable, even if the user implements the methods. + * So instead, we implement the methods wherein we throw exceptions. + * The reason we choose these methods is because: + * - If the user implements __serialize / __unserialize, the respective throwing methods are not called. + * - If the user implements __sleep / __wakeup, then it's also not a problem because they will not enter the throwing methods. + */ + +PHP_METHOD(DOMNode, __sleep) +{ + zend_throw_exception_ex(NULL, 0, "Serialization of '%s' is not allowed, unless serialization methods are implemented in a subclass", ZSTR_VAL(Z_OBJCE_P(ZEND_THIS)->name)); + RETURN_THROWS(); +} + +PHP_METHOD(DOMNode, __wakeup) +{ + zend_throw_exception_ex(NULL, 0, "Unserialization of '%s' is not allowed, unless unserialization methods are implemented in a subclass", ZSTR_VAL(Z_OBJCE_P(ZEND_THIS)->name)); + RETURN_THROWS(); +} + #endif diff --git a/ext/dom/php_dom.stub.php b/ext/dom/php_dom.stub.php index bbf7bad0f1491..3fb2909a6f922 100644 --- a/ext/dom/php_dom.stub.php +++ b/ext/dom/php_dom.stub.php @@ -56,7 +56,6 @@ public function after(...$nodes): void; public function replaceWith(...$nodes): void; } -/** @not-serializable */ class DOMNode { /** @readonly */ @@ -104,6 +103,10 @@ class DOMNode public string $textContent; + public function __sleep(): array {} + + public function __wakeup(): void {} + /** @return DOMNode|false */ public function appendChild(DOMNode $node) {} @@ -156,7 +159,6 @@ public function removeChild(DOMNode $child) {} public function replaceChild(DOMNode $node, DOMNode $child) {} } -/** @not-serializable */ class DOMNameSpaceNode { /** @readonly */ @@ -182,6 +184,12 @@ class DOMNameSpaceNode /** @readonly */ public ?DOMNode $parentNode; + + /** @implementation-alias DOMNode::__sleep */ + public function __sleep(): array {} + + /** @implementation-alias DOMNode::__wakeup */ + public function __wakeup(): void {} } class DOMImplementation diff --git a/ext/dom/php_dom_arginfo.h b/ext/dom/php_dom_arginfo.h index 1be65cb75d16b..edcc883aba57e 100644 --- a/ext/dom/php_dom_arginfo.h +++ b/ext/dom/php_dom_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: 20a0ff883af3bbf073d9c8bc8246646ffafe7818 */ + * Stub hash: 203760d1cf0e063ffd9abe743a0e24a97985767e */ ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(arginfo_dom_import_simplexml, 0, 1, DOMElement, 0) ZEND_ARG_TYPE_INFO(0, node, IS_OBJECT, 0) @@ -28,6 +28,11 @@ ZEND_END_ARG_INFO() #define arginfo_class_DOMChildNode_replaceWith arginfo_class_DOMParentNode_append +ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_DOMNode___sleep, 0, 0, IS_ARRAY, 0) +ZEND_END_ARG_INFO() + +#define arginfo_class_DOMNode___wakeup arginfo_class_DOMChildNode_remove + ZEND_BEGIN_ARG_INFO_EX(arginfo_class_DOMNode_appendChild, 0, 0, 1) ZEND_ARG_OBJ_INFO(0, node, DOMNode, 0) ZEND_END_ARG_INFO() @@ -100,6 +105,10 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_class_DOMNode_replaceChild, 0, 0, 2) ZEND_ARG_OBJ_INFO(0, child, DOMNode, 0) ZEND_END_ARG_INFO() +#define arginfo_class_DOMNameSpaceNode___sleep arginfo_class_DOMNode___sleep + +#define arginfo_class_DOMNameSpaceNode___wakeup arginfo_class_DOMChildNode_remove + ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_INFO_EX(arginfo_class_DOMImplementation_getFeature, 0, 2, IS_NEVER, 0) ZEND_ARG_TYPE_INFO(0, feature, IS_STRING, 0) ZEND_ARG_TYPE_INFO(0, version, IS_STRING, 0) @@ -491,6 +500,8 @@ ZEND_END_ARG_INFO() ZEND_FUNCTION(dom_import_simplexml); ZEND_METHOD(DOMCdataSection, __construct); ZEND_METHOD(DOMComment, __construct); +ZEND_METHOD(DOMNode, __sleep); +ZEND_METHOD(DOMNode, __wakeup); ZEND_METHOD(DOMNode, appendChild); ZEND_METHOD(DOMNode, C14N); ZEND_METHOD(DOMNode, C14NFile); @@ -672,6 +683,8 @@ static const zend_function_entry class_DOMChildNode_methods[] = { static const zend_function_entry class_DOMNode_methods[] = { + ZEND_ME(DOMNode, __sleep, arginfo_class_DOMNode___sleep, ZEND_ACC_PUBLIC) + ZEND_ME(DOMNode, __wakeup, arginfo_class_DOMNode___wakeup, ZEND_ACC_PUBLIC) ZEND_ME(DOMNode, appendChild, arginfo_class_DOMNode_appendChild, ZEND_ACC_PUBLIC) ZEND_ME(DOMNode, C14N, arginfo_class_DOMNode_C14N, ZEND_ACC_PUBLIC) ZEND_ME(DOMNode, C14NFile, arginfo_class_DOMNode_C14NFile, ZEND_ACC_PUBLIC) @@ -694,6 +707,8 @@ static const zend_function_entry class_DOMNode_methods[] = { static const zend_function_entry class_DOMNameSpaceNode_methods[] = { + ZEND_MALIAS(DOMNode, __sleep, __sleep, arginfo_class_DOMNameSpaceNode___sleep, ZEND_ACC_PUBLIC) + ZEND_MALIAS(DOMNode, __wakeup, __wakeup, arginfo_class_DOMNameSpaceNode___wakeup, ZEND_ACC_PUBLIC) ZEND_FE_END }; @@ -989,7 +1004,6 @@ static zend_class_entry *register_class_DOMNode(void) INIT_CLASS_ENTRY(ce, "DOMNode", class_DOMNode_methods); class_entry = zend_register_internal_class_ex(&ce, NULL); - class_entry->ce_flags |= ZEND_ACC_NOT_SERIALIZABLE; zval property_nodeName_default_value; ZVAL_UNDEF(&property_nodeName_default_value); @@ -1104,7 +1118,6 @@ static zend_class_entry *register_class_DOMNameSpaceNode(void) INIT_CLASS_ENTRY(ce, "DOMNameSpaceNode", class_DOMNameSpaceNode_methods); class_entry = zend_register_internal_class_ex(&ce, NULL); - class_entry->ce_flags |= ZEND_ACC_NOT_SERIALIZABLE; zval property_nodeName_default_value; ZVAL_UNDEF(&property_nodeName_default_value); diff --git a/ext/dom/tests/gh8996.phpt b/ext/dom/tests/gh8996.phpt new file mode 100644 index 0000000000000..62b7955bacf11 --- /dev/null +++ b/ext/dom/tests/gh8996.phpt @@ -0,0 +1,120 @@ +--TEST-- +GH-8996: DOMNode serialization on PHP ^8.1 +--EXTENSIONS-- +dom +--FILE-- +xmlData = $this->saveXML(); + return ['xmlData']; + } + + public function __wakeup(): void + { + $this->loadXML($this->xmlData); + } +} + +$dom = new SerializableDomDocumentSleepWakeup('1.0', 'UTF-8'); +$dom->loadXML('value'); + +$serialized = serialize($dom); +var_dump($serialized); +$unserialized = unserialize($serialized); + +echo "Serialized:\n-----------\n$serialized\n-----------\nRestored:\n-----------\n{$unserialized->saveXml()}"; + +echo "=== __serialize and __unserialize ===\n"; + +class SerializableDomDocument__Serialize__Unserialize extends DOMDocument +{ + public function __serialize(): array + { + return ['xmlData' => $this->saveXML()]; + } + + public function __unserialize(array $data): void + { + $this->loadXML($data['xmlData']); + } +} + +$dom = new SerializableDomDocument__Serialize__Unserialize('1.0', 'UTF-8'); +$dom->loadXML('value'); + +$serialized = serialize($dom); +$unserialized = unserialize($serialized); + +echo "Serialized:\n-----------\n$serialized\n-----------\nRestored:\n-----------\n{$unserialized->saveXml()}"; + +echo "=== serialize and unserialize ===\n"; + +class SerializableDomDocumentSerializeUnserialize extends DOMDocument implements Serializable +{ + public function serialize(): ?string + { + return $this->saveXML(); + } + + public function unserialize(string $data): void + { + $this->loadXML($data); + } +} + +$dom = new SerializableDomDocumentSerializeUnserialize('1.0', 'UTF-8'); +$dom->loadXML('value'); + +$serialized = serialize($dom); +$unserialized = unserialize($serialized); + +echo "Serialized:\n-----------\n$serialized\n-----------\nRestored:\n-----------\n{$unserialized->saveXml()}"; + +?> +--EXPECTF-- +=== __sleep and __wakeup === +string(144) "O:34:"SerializableDomDocumentSleepWakeup":1:{s:43:"%0SerializableDomDocumentSleepWakeup%0xmlData";s:39:" +value +";}" +Serialized: +----------- +O:34:"SerializableDomDocumentSleepWakeup":1:{s:43:"%0SerializableDomDocumentSleepWakeup%0xmlData";s:39:" +value +";} +----------- +Restored: +----------- + +value +=== __serialize and __unserialize === +Serialized: +----------- +O:47:"SerializableDomDocument__Serialize__Unserialize":1:{s:7:"xmlData";s:39:" +value +";} +----------- +Restored: +----------- + +value +=== serialize and unserialize === + +Deprecated: SerializableDomDocumentSerializeUnserialize implements the Serializable interface, which is deprecated. Implement __serialize() and __unserialize() instead (or in addition, if support for old PHP versions is necessary) in %s on line %d +Serialized: +----------- +C:43:"SerializableDomDocumentSerializeUnserialize":39:{ +value +} +----------- +Restored: +----------- + +value diff --git a/ext/dom/tests/not_serializable.phpt b/ext/dom/tests/not_serializable.phpt index 9869a8c87e35a..3d542861db0aa 100644 --- a/ext/dom/tests/not_serializable.phpt +++ b/ext/dom/tests/not_serializable.phpt @@ -36,7 +36,7 @@ try { ?> --EXPECT-- -Serialization of 'DOMDocument' is not allowed -Serialization of 'DOMElement' is not allowed +Serialization of 'DOMDocument' is not allowed, unless serialization methods are implemented in a subclass +Serialization of 'DOMElement' is not allowed, unless serialization methods are implemented in a subclass Serialization of 'DOMXPath' is not allowed -Serialization of 'DOMNameSpaceNode' is not allowed +Serialization of 'DOMNameSpaceNode' is not allowed, unless serialization methods are implemented in a subclass diff --git a/ext/dom/tests/not_unserializable.phpt b/ext/dom/tests/not_unserializable.phpt new file mode 100644 index 0000000000000..a25a2737e852d --- /dev/null +++ b/ext/dom/tests/not_unserializable.phpt @@ -0,0 +1,29 @@ +--TEST-- +DOM classes are not unserializable +--EXTENSIONS-- +dom +--FILE-- +getMessage(), "\n"; + } +} + +?> +--EXPECT-- +Unserialization of 'DOMXPath' is not allowed +Unserialization of 'DOMDocument' is not allowed, unless unserialization methods are implemented in a subclass +Unserialization of 'DOMNode' is not allowed, unless unserialization methods are implemented in a subclass +Unserialization of 'DOMNameSpaceNode' is not allowed, unless unserialization methods are implemented in a subclass