Fix GH-14402: Add support for serialization in SplPriorityQueue, SplMinHeap and SplMaxHeap

[alexandre-daubois]

Fix #14402

[TimWolla]

I believe the serialization format is unsafe when facing inheritance, since it doesn't clearly distinguish between “regular properties” and “internal state”. Please have a look at the serialization implementation of ext/random engines for a robust example and make sure to include proper error handling (i.e. throwing an \Exception) when the data does not look as expected during unserialization. Silently doing nothing when the zval types do not match is not okay.

[alexandre-daubois]

I pushed a new version with 3 additional tests:

• A test to check that __unserialize() throws proper exceptions for malformed data
• A test that verifies user properties with conflicting names (like $flags, $heap_elements) don't interfere with the internal state
• And a last one that confirms the new indexed array format [properties, internal_state] is used consistently across heap classes. The same pattern is used in PHP_METHOD(Random_Engine_Mt19937, __unserialize) for example. Thanks for pointing out!

Tests I pushed are pretty verbose and human friendly, I wonder if they should look like ext/spl/tests/SplObjectStorage_unserialize_nested.phpt for example, with the whole internal structure?

[alexandre-daubois]

I addressed all your comments in 3f041a3

[TimWolla]

Should we prevent serialization if the SPL_HEAP_CORRUPTED flag is set?

I'm also noticing that this PR targets PHP 8.3. I'd argue this is more a feature rather than a bugfix and would recommend targeting master.

[TimWolla]

Tests I pushed are pretty verbose and human friendly, I wonder if they should look like ext/spl/tests/SplObjectStorage_unserialize_nested.phpt for example, with the whole internal structure?

Maintenance-wise I think “technical tests” are preferable, this makes sure that they capture every nuance and with the diff output any failures are still pretty readable.

You could also take a look at the serialization tests for ext/random. Given that it's a very new extension, I'd say it's in a good shape. And I made sure to very carefully maintain the tests there.

[alexandre-daubois]

I'm also noticing that this PR targets PHP 8.3. I'd argue this is more a feature rather than a bugfix and would recommend targeting master.

Yes. I targeted 8.3 because of the bug label on the issue, but it definitely looks like a feature. Let's target master.

[alexandre-daubois]

The two last commits rearrange tests with a more technical approach, adds some more tests and safety about the heap being corrupted. Also, more code form Random_Engine_Mt19937 is borrowed.

[TimWolla]

The logic now looks robust to me (didn't look at the test). But as I'm not familiar with the code, I would like to see someone else do a review.

[TimWolla]

For this one you should throw to wrap the exception into the “Invalid serialization data for %s object” one. When throwing while an exception is already active, the engine will automatically set $previous:

Suggested change

	RETURN_THROWS();
	zend_throw_exception_ex(NULL, 0, "Invalid serialization data for %s object", ZSTR_VAL(intern->std.ce->name));
	RETURN_THROWS();

[alexandre-daubois]

In case object_properties_load() sets one in the executor globals, alright I get it. PR updated

[TimWolla]

ditto

Resolved

[alexandre-daubois]

Thanks for the extensive review @TimWolla!