Conversation
TysonAndre
left a comment
TysonAndre
left a comment
There was a problem hiding this comment.
LGTM other than the question about not removing zend_class_serialize_deny /zend_class_unserialize_deny
|
|
||
| if (UNEXPECTED((decl->flags & ZEND_ACC_ANON_CLASS))) { | ||
| /* Serialization is not supported for anonymous classes */ | ||
| ce->serialize = zend_class_serialize_deny; |
There was a problem hiding this comment.
I think it'd be safer to add ZEND_ACC_NOT_SERIALIZABLE but keep zend_class_serialize_deny until the next minor release, especially with the feature freeze in a few days.
Older releases of msgpack and igbinary (and possibly others) would depend on zend_class_serialize_deny and zend_class_unserialize_deny to work correctly, and some of them would have releases that would compile in 8.1.0+ stable or be used to beta test 8.1
(e.g. to avoid unserializing an invalid Closure)
There was a problem hiding this comment.
@krakjoe Thoughts on this? I'd rather not duplicate the info, and we're still half a year away from the release.
There was a problem hiding this comment.
I don't like duplication either, and I don't like leaving broken API exposed for longer than we must.
|
It'd be useful to add a test that the bug is fixed. Fixing all class entries can be left for a subsequent PR, but in order for php, igbinary, msgpack, etc. to be able to test this and avoid regressions, it'd be useful to change at least one non-final class entry. --- a/ext/curl/curl_file.c
+++ b/ext/curl/curl_file.c
@@ -149,6 +149,7 @@ void curlfile_register_class(void)
curl_CURLFile_class = register_class_CURLFile();
curl_CURLFile_class->serialize = zend_class_serialize_deny;
curl_CURLFile_class->unserialize = zend_class_unserialize_deny;
+ curl_CURLFile_class->ce_flags |= ZEND_ACC_NOT_SERIALIZABLE;
curl_CURLStringFile_class = register_class_CURLStringFile();
}--TEST--
Test refusing to serialize/unserialize unserializable classes
--INI--
error_reporting=E_ALL
--FILE--
<?php
// https://bugs.php.net/bug.php?id=81111
function check_serialize_throws($obj) {
try {
var_dump(serialize($obj));
} catch (Throwable $e) {
echo "Caught: " . $e->getMessage() . "\n";
}
}
class Something extends CURLFile {
public function __serialize() { return []; }
public function __unserialize($value) { return new self('file'); }
}
check_serialize_throws(new Something('file'));
check_serialize_throws(new class () {
public function __serialize() { return []; }
public function __unserialize($value) { }
});
?>
--EXPECTF--
Caught: Serialization of 'Something' is not allowed
Caught: Serialization of 'class@anonymous' is not allowed |
|
@nikic can you merge this today, so we have time for follow ups please. |
|
LGTM |
This prevents serialization and unserialization of a class and its children in a way that does not depend on the zend_class_serialize_deny and zend_class_unserialize_deny handlers that will be going away in PHP 9 together with the Serializable interface. In stubs, `@not-serializable` can be used to set this flag. This patch only uses the new flag for a handful of Zend classes, converting the remainder is left for later.
nikic
force-pushed
the
deny-serialization
branch
from
b244906 to
867bcfc
Compare
4 participants
This prevents serialization and unserialization of a class and its
children in a way that does not depend on the zend_class_serialize_deny
and zend_class_unserialize_deny handlers that will be going away
in PHP 9 together with the Serializable interface.
In stubs,
@not-serializablecan be used to set this flag.This patch only uses the new flag for a handful of Zend classes,
converting the remainder is left for later.