Opis\Closure enters an unusable state if used in try/catch

[brentkelly]

I have the following use case:

• $foo may contain data that is either serialized or unserialized.
• If it is serialized, I need to unserialize to ensure $foo is now in a consistent, unserialized state.

Best I can tell there is no official way to detect if something can be unserialized. So the way I had implemented was in a seemingly-innocent try/catch block:

$bar = $foo;
try {
    $bar = \Opis\Closure\unserialize($foo);
} catch (Exception $e) {}

Now $bar is always unserialized.

However this now causes \Opis\Closure to enter essentially a broken state if the try block fails. As soon as that occurs, the locks depth used in \Opis\Closure\SerializableContext::enterContext will now never reach zero, causing all future serializations to happen in the same context. There is no way to forcibly reset the context state either.

This creates some extremely random bugs - in my situation certain tests late in the test suite fail because when attempting to serialize an object instance using \Opis\Closure\serialize, \Opis\Closure\SerializableClosure::wrapClosures updates the instance by reference to become simply boolean true. This happens because we are attempting to serialize inside the context of a previous serialization ... but only if specific other tests earlier in the suite were run first. The tests are bleeding into each other due to \Opis\Closure's essentially global state.

In a wider use case, its not unforeseeable that someone would try unserializing in a much wider try/catch block than my example above. If it falls over and their code execution continues, it is going to produce some very bizarre & hard-to-track-down bugs.

So best I can tell I'm stuck with the current implementation. For now I've just created a class that extends \Opis\Closure\SerializableClosure and provides a clean method:

/**
 * Restore Opis\Closure to a clean state, read for the next call to
 * serialize/unserialize
 *
 * @return void
 */
public static function clean()
{
    static::$context = null;
}

Now I can clean up as required:

use Foo\Custom\SerializableClosure;

$bar = $foo;
try {
    $bar = \Foo\Custom\unserialize($foo);
} catch (Exception $e) {
    SerializableClosure::clean();
}

Now it cleans up after itself & allows future serialization.

I'm mainly just reporting this in case its of use to you guys - but a couple of suggestions for your consideration:

1. Ideally use a try/catch when serializing/unserializing & clean up the global state (before rethowing) in a catch block.
2. Alternatively provide a clean method, or some other way to restore to a clean state / forcibly exit the current context as I've done above
3. Perhaps you could consider adding an additional doc under "Good to know" or at least a note to https://opis.io/closure/3.x/context.html.

[jonathonsim]

Perhaps unserialize itself should wrap the unserialization in a try/finally so it can ensure the context is exited before passing any exceptions on
eg in functions.php

/**
 * Unserialize
 *
 * @param string $data
 * @param array|null $options
 * @return mixed
 */
function unserialize($data, array $options = null)
{
    SerializableClosure::enterContext();
   try {
    $data = ($options === null || \PHP_MAJOR_VERSION < 7)
        ? \unserialize($data)
        : \unserialize($data, $options);
    SerializableClosure::unwrapClosures($data);
    }
    finally  {
       SerializableClosure::exitContext();
    }
    return $data;
}

(BTW, I had to check for myself that finally runs here before passing that exception on in the absence of a catch : I can confirm it does behave as expected and run the exitContext in that case before then passing that exception on as-per normal. For some reason in all my years of PHP of never really come across that requirement before and the docs are vague on the subject
)