Hi
On 9/28/22 19:53, Nicolas Grekas wrote:
I'm not quoting the full text of the discussion because the php ML server
refused the message - too long it said.
Yeah, that's fine. I also aggressively prune irrelevant parts from
whatever I quote; saves traffic and storage, and also makes it easier to
find the answers. Additional context is available by looking up the
quoted message.
Based on my analysis and understanding of the linked examples, the only
one that *might* break (I don't fully understand it), is the one in
src/Symfony/Component/VarExporter/Internal/Registry.php. The others will
either no change or change to be more correct.
[…]
As you saw, the BC break is real. That's what I wanted to highlight and
what worries me for the community at large. This is not some niche BC break.
That's not at all what I'd say I've seen. My argument (see quote) was
that while it *technically* breaks BC under a strict definition, it does
not change behavior in a *negative way*. I don't find this strict
definition of BC break particularly useful, because that effectively
results in https://xkcd.com/1172/ ("Workflow"), preventing all progress.
There is also an argument that is missing in this conversation and that
Yasuo made (in another thread by mistake I guess?):
Yes, I have seen that one. For reference of the other readers, the email
is here: https://externals.io/message/118554#118684.
In general, unserializing external serialised data is the same as "Read and
execute remote PHP code"
(Executing arbitrary code via memory corruption is trivial task with
unserialize())
I am aware of that and acknowledged the security issue in the third list
point of this section:
https://wiki.php.net/rfc/improve_unserialize_error_handling#increase_the_error_reporting_severity_in_the_unserialize_parser
In all valid use cases of unserialize(), we do trust the backend where we
get serialized payloads from. This means constructed broken payloads are
not something we need to deal with (the DateTimeImmutable example you
gave).
Okay, that's fair with regard to DTI. The 'Error' case still exists for
readonly classes that lose properties, so ...
The thing we need to guard against is much narrower. That makes the problem
this PR aims to solve much smaller. Truncated payloads are a thing because
of race conditions or unreliable network services. FC/BC of serialized
payloads is another thing that can only be dealt with by implementing
__unserialize() or the likes. And that's mostly it. Other errors (e.g.
... while it is true that this FC/BC of payloads needs to be dealt with
by implementing __unserialize(), you can't rely on every class doing
that (correctly). That's why you currently need to catch 'Throwable',
not just 'Exception' and that's why the RFC proposes to wrap the Throwables.
Truncated payloads should not happen even with unreliable network
services, because of integrity checks embedded into the protocol (e.g.
TLS). In any case, truncated payloads will currently emit E_NOTICE, so
that relates to the second vote (E_WARNING to Exception) more than the
first vote (wrap Exception), whereas I understand your concerns are
mostly with the "wrap Exception" part.
parse error) are not caused by unserialize itself, but by regular user code.
Sorry, I don't understand what "parse error" or "other errors" you are
referring to.
"RFC: Static variables in inherited methods" -
https://wiki.php.net/rfc/static_variable_inheritance
That RFC actually changed the behavior of the application I maintain.
You voted in favor of the RFC, but did not participate in the
discussion. The fix was simple and I agree that the new behavior is
better, so no hard feelings. I'd be curious though how the breakage from
that RFC is more acceptable to you than the anticipated breakage from
this RFC? Honest question to understand your PoV better.
Rereading the RFC, Nikita described a niche and obscure behavior of the
engine. There are no numbers to support that understanding (how "niche"
this was) and that's missing in the RFC. I remember other RFCs which did
have numbers to quantify the impact. I guess it was hard to get that
Understood, thank you for looking into this. I believe raw numbers are
not the (most) important metric. IMO subtlety of the break is more
important. A subtle change that affects a small minority is worse than a
less subtle change that affects more users.
number. And that's also why I made this gist: so that the impact of the BC
break could be evaluated somehow. Many spots in Symfony likely means many
more spots in the PHP community. I'm not going to be able to conduct a
I didn't and don't expect you to perform a larger-scale analysis, that's
hard to do, because one has to look at the *context* of the
'unserialize()' call to determine what exactly the intended behavior is.
That's what I attempted to do with your Symfony examples (I am not a
Symfony user and might have misunderstood some, though) and based on
that I disagree with "many spots".
For the application I maintain at work, I've also looked through the
unserialize() calls and concluded the the vast majority of them are
buggy as they already are and I am planning to fix those independent of
the results of this RFC.
larger analysis of this impact, but that can still give an intuition of it
to readers. Mine is: it's likely going to be impactful, not in a good way.
Best regards
Tim Düsterhus
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
