On Fri, 29 Jul 2022 at 18:17, Tim Düsterhus <tim@bastelstu.be> wrote:
> Hi!
>
> I'm currently in the process of cleaning up the error handling of the
> new ext/random and now came across the implementation of __unserialize().
>
> Looking through the source code for existing examples I found that the
> following is used:
>
> - ext/gmp: Exception
> - ext/hash: Exception
> - ext/date: Error
> - ext/spl: UnexpectedValueException
> - ext/random: Currently Exception.
>
> … all of those with different error messages. unserialize() itself will
> emit a regular 'Notice' (yes, you read that right) when encountering
> garbage data.
>
> In the end I've opted to follow ext/date's lead and created a PR
> changing ext/random to use an Error, as unserialization failures likely
> mean that you are unserializing untrusted data which you should not do
> in the first place. Also any errors during unserializing are not really
> recoverable: You can't go and fix up the input string.
>
> The PR can be found here:
>
> https://github.com/php/php-src/pull/9185
>
> During review cmb noted that using an 'Error' here might not be the best
> choice, as while it is likely to be a programmer error if unserializing
> fails, we do not want to educate users to catch(Error).
>
I probably have a very different philosophy on this as I don't mind users
catching Errors, but it probably be mostly done by Framework/libraries
doing some funky stuff.
And passing user input which can fail to unserialize is not really a
programming error and something that can be "expected" to happen, I agree
that an Exception does make more sense here.
> As the existing behavior already is inconsistent and the Notice really
> should be upgraded to something … more alarming in the future, I'm
> putting this topic up for discussion on the list.
>
> My suggested path forward would be:
>
> For 8.2:
>
> - Update ext/random to use the ext/date wording (I like that one best):
>
> "Invalid serialization data for <classname> object."
>
> - Revert the change from Exception to Error in my ext/random PR #9185.
>
Those make sense to me
> - Update ext/date to Exception (the Error for __unserialize() was added
> in 8.2, but there already was an Error for __wakeup() before).
>
I'm not a super fan of this, both __wakeup and __unserialize should use the
same one.
As far as I can tell the reason __wakeup() is still defined is if someone
called it explicitly.
But in any case if someone was handing an unserialisation failure (that was
handled by __wakeup prior to 8.2) and caugh an Error explicitly this would
now break.
Thus I'd prefer handling this with the rest after 8.2
> For whatever comes after 8.2:
>
> - Add a new UnserializationFailureException extends Exception (or
> whatever color the bikeshed should have). Any existing catch blocks
> should still match, as Exception is more general. It would break for
> ext/spl, though. Unless UnserializationFailureException extends
> UnexpectedValueException.
>
As we would be breaking BC with ext/date anyway I think having it just
extend Exception would be fine
> - Add a helper function that throws this Exception with a consistent
> wording.
> - Upgrade unserialize() from Notice to UnserializationFailureException.
>
Notice to Exception might be a big jump, E_WARNING definitely an then
promote to an Exception in 9.0 is probably more in line with what we did
for 7.x/8.0
George P. Banyard
