Hi
On 10/18/22 03:26, Tim Starling wrote:
If I'm reading the implementation correctly, the original exception is
thrown away, there's no way to get the original message and backtrace.
That will make debugging more difficult.
See Derick's reply (and my reply to Derick's reply).
I reviewed about 150 callers of unserialize() in the MediaWiki core
and extensions. There were no instances of the kind of error guarding
shown in the RFC. Occasionally, we have:
$result = @unserialize( $data );
if ( !$result ) {
throw new Exception( ... );
}
The assumption is that if there is a failure, false will be returned,
as is documented in the manual. I don't understand the assertion in
the RFC that set_error_handler() is required to detect unserialization
failure.
false is a valid value that might've been serialized. The manual
specifically notes:
false is returned both in the case of an error and if unserializing the serialized false value. It is possible to catch this special case by comparing data with serialize(false) or by catching the issued E_NOTICE.
Note that your example code also errors out if the $result is an empty
array, an empty string or another falsy values. This might be an
acceptable result in your specific case, but is not the correct behavior
in the general case.
The example in the RFC was written to be correct for the general case,
without imposing any constraints in the input data.
Normally, exceptions in MediaWiki are allowed to propagate back to the
global exception handler. There are a couple of categories of
exceptions which should never be caught as a matter of policy:
* Database errors. These are permanent errors. Attempting to persist
with executing the request after a database error will mostly likely
lead to another error.
* Request timeout exceptions. We're using our Excimer extension to
implement request timeouts by throwing exceptions from a timer
callback. This has been an interesting can of worms which has brought
some nice features with a number of unexpected consequences. It means
that it's always incorrect to catch and discard an arbitrary Throwable.
Please note that this is already broken independently of this RFC. It is
entirely reasonable for a library to catch all Exceptions that happen
internally and wrap them in a well-defined "library exception" at the
library boundary.
As a specific example, the "flysystem" file system abstraction library
is doing this:
https://github.com/thephpleague/flysystem/blob/f5e93fad64cff5f45a1f79246dae15b31099b105/src/MountManager.php#L35-L37
Probably nothing in our ecosystem is doing database access in an
__unserialize() magic method. But if someone tried it, I would want
the resulting errors to be properly logged with correct backtraces,
not silently discarded.
The internal behavior of unserialization is unspecified. It only
guarantees you that you get back your original structure and that all
the unserialization callbacks will have been called if unserialization
fails. For the error case there are no guarantees, except "a Throwable
might be thrown".
As an example PHP 7.0.10 changed the behavior for unserialization
failures to no longer call __destruct() on incompletely initialized
objects to fix a security issue:
https://3v4l.org/82UJj
I believe this is the corresponding bug:
https://bugs.php.net/bug.php?id=72663
In PHP 7.0.15/7.1.1 apparently there was another change:
https://3v4l.org/tnh2R
As such you can't rely on the database access Exception to actually be
emitted if unserialization of another object also fails.
Best regards
Tim Düsterhus
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
