Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:128660
Message-ID: <b7d35ecd-d68a-45e8-be88-792a15b3d79b@bastelstu.be>
Date: Mon, 8 Sep 2025 22:44:33 +0200
Precedence: list
MIME-Version: 1.0
Subject: Re: [PHP-DEV] [RFC] Soft-Deprecate __sleep() and __wakeup()
To: Jakub Zelenka <bukka@php.net>
Cc: Nicolas Grekas <nicolas.grekas+php@gmail.com>,
 PHP Internals List <internals@lists.php.net>
References: <CAOWwgpk1Z_bns=Z55TqpYOBXvBGpurMfjTGPSgQ6Uf5QXfLOjg@mail.gmail.com>
 <cddcbba0366184b616e34637b424645e@bastelstu.be>
 <CAEKnhAFFgpBSOU+yTAfvY6_ndZwOmKvOKZG=F2+mamqkJPfrrQ@mail.gmail.com>
Content-Language: en-US
In-Reply-To: <CAEKnhAFFgpBSOU+yTAfvY6_ndZwOmKvOKZG=F2+mamqkJPfrrQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=C3=BCsterhus?=)

Hi

On 9/8/25 17:32, Jakub Zelenka wrote:
> I think the point here was that it was close and the RFC itself was

Again, a 2/3 majority is *not* close. Twice the number of folks were in 
favor than against the RFC.

> misleading and omitted some important points that would like change the
> final result.

I concede that “straight up improvement” might not be perfectly accurate 
for `__wakeup()`, but for `__sleep()` it *is* accurate. `__sleep()` is 
broken and there's a straight-forward migration to `__serialize()`.

I appreciate that both of you communicated your concerns with the 
proposed deprecation during the discussion, but you basically just said 
“I believe the impact is too large”, which is not actionable feedback 
for the RFC author and also doesn't help voters make an educated 
decision. The point of the discussion phase is figuring out these 
details together.

>> The serialization mechanism is also a security sensitive part of the
>> language, the fewer moving parts there are, the better. Security is part
>> of the motivation for me.
>>
>>
> Could you be more specific here? We do not consider issues (crashes and
> similar) resulting from unserializing of the serialized string as security
> issues because it must not come from the untrusted source (see
> https://www.php.net/manual/en/function.unserialize.php ). I don't remember
> any security issue in serialize / unserialize since this rule was set.

Even when only dealing with trusted data, unserialize needs to parse 
inputs (which is already sufficiently dangerous in C) and it also needs 
to deal with partial(ly initialized) object graphs all while executing 
arbitrary (userland) code that may or may not interact in weird ways 
(e.g. when error handlers get involved due to a bug in a custom 
unserialize hook). And of course there can also be bugs with the 
serializer that result in “trusted” output that triggers unexpected and 
untested code-paths, particularly when references or cycles get 
involved. When running with mixed PHP versions during an incremental 
upgrade it might also be possible for serialization data to be emitted 
by newer PHP versions that may not be anticipated by older PHP versions 
and that trigger unexpected code-paths.

And then there's also stuff like 
https://wiki.php.net/rfc/unserialize_warn_on_trailing_data, which can 
happen due to bugs in a userland implementation even when not doing 
unserialize($_GET['stuff']).

Saying that “unserialize is not security-relevant because you must only 
feed it safe inputs” as an excuse to avoid making unserialize safer is 
not helping anyone and is downplaying the risks involved in unserialization.

For all those reasons, I've intentionally tried to make the 
unserialization hooks as robust as possible during review of newly added 
classes (e.g. ext/random, ext/uri, and also 
https://github.com/php/php-src/pull/19447), making them carefully check 
the structure of the serialization input and also having tests that 
verify robustness even when facing untrusted inputs, because that's just 
something that will happen in the real world, despite the documentation 
advising against it.

Best regards
Tim Düsterhus