Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118311
Message-ID: <302000df-5c3f-a86c-a608-2a45d2726ab1@bastelstu.be>
Date: Fri, 29 Jul 2022 19:17:33 +0200
MIME-Version: 1.0
Content-Language: en-US
To: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: What type of Exception to use for unserialize() failure?
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=c3=bcsterhus?=)

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).

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.
- Update ext/date to Exception (the Error for __unserialize() was added 
in 8.2, but there already was an Error for __wakeup() before).

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.
- Add a helper function that throws this Exception with a consistent 
wording.
- Upgrade unserialize() from Notice to UnserializationFailureException.

Opinions?

Best regards
Tim Düsterhus