Re: RFC [Discussion]: Improve unserialize() error handling

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