Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118831
MIME-Version: 1.0
References: <22177032-fe72-c39b-63fe-fa4368a70852@bastelstu.be>
 <CAOWwgpmuiRzUwGpU3ShQ7HOAVGhcP=Wb8z50acLfERMn_NEbwQ@mail.gmail.com>
 <c9e61189-7826-844f-56b9-7bb509a0791d@bastelstu.be> <CAOWwgpnpBTx9-aLz-bg1xZ9FT5JyWsX44TLwfvanDS_aAW7qdQ@mail.gmail.com>
 <c69c45f7-9836-1858-d90e-df7042bd3f88@bastelstu.be>
In-Reply-To: <c69c45f7-9836-1858-d90e-df7042bd3f88@bastelstu.be>
Date: Mon, 17 Oct 2022 10:57:21 +0200
Message-ID: <CAOWwgpmZ82zp7apmrwmb+xKOrwJvMuOJ=OMBuZa3n44ZPX5Uwg@mail.gmail.com>
To: =?UTF-8?Q?Tim_D=C3=BCsterhus?= <tim@bastelstu.be>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000008470dc05eb372ac7"
Subject: Re: [PHP-DEV] [VOTE] Improve unserialize() error handling
From: nicolas.grekas+php@gmail.com (Nicolas Grekas)

--0000000000008470dc05eb372ac7
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Tim,

Thanks for taking the time to have a closer look. Unfortunately, you picked
the one failing test where there could be a discussion about whether the
original code needs improvement or not.

The other failing tests involve "unserialize_callback_func" to gracefully
detect class-not-found, and they are all plain broken by the RFC.

I created this patch/PR to show the changes that would be required on
Symfony to work around the BC break:
https://github.com/symfony/symfony/pull/47882

Note to readers: in this whole discussion, Symfony is just an example of
affected code. In the end, Symfony will adapt to the RFC if it passes. The
point being made is that PHP scripts should not have to be patched to run
on newer minor versions of PHP. That's what "keeping BC" means.


Resending the message without attachments, because the mailing list
> rejected the original due to exceeding 30000 bytes:
>
> > ezmlm-reject: fatal: Sorry, I don't accept messages larger than 30000
> bytes (#5.2.3)
>
> You can find the attachments at:
> https://gist.github.com/TimWolla/376dd162f7684daef38f76a07254871c
>
> Original message below:
>
> ------------------
>
> On 10/15/22 11:06, Nicolas Grekas wrote:
> >> I'm not surprised by the =E2=80=9Cno=E2=80=9D on the first vote based =
on the previous
> >> discussion. I am surprised however that you also disagree with raising
> >> the E_NOTICE to E_WARNING for consistency.
> >>
> >> I don't plan on repeating all the previous discussion points with you,
> >> but as you mention the Symfony tests specifically: Please find the pat=
ch
> >> attached. Do you believe the expectations in this new test would a
> >> reasonable expectation by a Symfony user?
> >>
> >
> > Since the beginning my point is not that the RFC doesn't have merits.
> It's
> > that the proposed approach breaks BC in a way that will affect the
> > community significantly. We have policies that say we should avoid BC
> > breaks and they should apply here.
> >
> > To anyone wondering about the reality of the BC break if you're not
> > convinced there is one because the original code is broken anyway (whic=
h
> is
> > your main argument Tim IIUC): please have a look at the failures I
> reported
>
> Yes, you understood this correctly: I consider the existing
> implementation to be broken. Unfortunately you did not answer my
> question whether the attached patch would be a reasonable expectation by
> a Symfony user.
>
> Let's presume it is: If I use the 'PhpSerializer' then I expect to
> receive exactly the 'MessageDecodingFailedException' whenever the
> message fails to decode. When decoding an erroneous DateTime (or an
> erroneous SplDoublyLinkedList or whatever) a different Exception will be
> thrown and not caught, thus the contract by the 'PhpSerializer' is
> violated.
>
> Let's fix this using test driven development. We first add the failing
> test cases. This is step1.diff (patches are in
> https://gist.github.com/TimWolla/376dd162f7684daef38f76a07254871c). When
> running the tests we get the following output:
>

I didn't answer the question because we ruled out such payloads in the
discussion thread: constructed payloads like the ones involving DateTime do
not exist in practice. Or if they do, it means the source of the payload is
not trusted, and then we are in much bigger trouble (security issues for
which the RFC can't do anything.)

Breaking BC in the name of such invalid payloads doesn't make an argument
in favor of the RFC.


> Failed asserting that exception message 'Could not decode message using
> PHP serialization: O:13:"ReceivedSt0mp":0:{}.' matches '/class
> "ReceivedSt0mp" not found/'.
>
> Okay, now the Exception message changed. Personally I do not consider
> this a BC break: I believe Exception messages are meant for human
> consumption, not for programs. Otherwise fixing a typo in the message
> would be a BC break. If the code wants to learn about the cause, it
> should either use the '$code' or different types of Exception should be
> thrown to clarify the cause by entering a different catch() block.
>

Yes, the specific error message should be part of the BC promise. This
allows building test suites that can assert the message in a stable way.
This is also why we don't change the output of var_dump/print_r/var_export:
they're written now, in the same of BC, for the best of stability. (I've
barely read any PHP code where exception's code is used in any useful BTW -
that can't be a solution.)

More importantly, the type of thrown exceptions should undoubtedly be part
of the BC promise.
Wrapping exceptions inside UnserializationFailedException breaks existing
catch/instanceof checks (see my PR above.)



> All green! We **did not have to change anything** after fixing the
> implementation in step3a.diff for PHP 8.2 and earlier!
>

We did change the exception hierarchy, both in terms of types and in terms
of stack of "previous". In this specific case, we can discuss the merits of
wrapping all throwables or not. But in doubt the other failing cases I
reported clearly highlight the BC break.

Nicolas

--0000000000008470dc05eb372ac7--