Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 4 commits into from

Conversation

alexandre-daubois
Copy link
Member

Fix #14402

@alexandre-daubois alexandre-daubois changed the base branch from master to PHP-8.3 August 11, 2025 13:01
TimWolla
TimWolla previously requested changes Aug 11, 2025
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

alexandre-daubois commented Aug 11, 2025 •
edited
Loading

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?

TimWolla
TimWolla reviewed Aug 11, 2025
View reviewed changes
@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Aug 12, 2025 •
edited
Loading

I addressed all your comments in 3f041a3

TimWolla
TimWolla reviewed Aug 12, 2025
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Member Author

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 alexandre-daubois changed the base branch from PHP-8.3 to master August 12, 2025 13:19
TimWolla
TimWolla reviewed Aug 12, 2025
View reviewed changes
@alexandre-daubois alexandre-daubois force-pushed the spl-heap-serialize branch 3 times, most recently from 4478ef7 to bb54148 Compare August 12, 2025 14:11
@alexandre-daubois
Copy link
Member Author

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
TimWolla reviewed Aug 12, 2025
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ext/spl/spl_heap.c

object_properties_load(&intern->std, Z_ARRVAL_P(props));
if (EG(exception)) {
RETURN_THROWS();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ext/spl/spl_heap.c

object_properties_load(&intern->std, Z_ARRVAL_P(props));
if (EG(exception)) {
RETURN_THROWS();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@TimWolla TimWolla dismissed their stale review August 12, 2025 14:16

Resolved

@alexandre-daubois
Copy link
Member Author

Thanks for the extensive review @TimWolla!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimWolla TimWolla TimWolla left review comments

@kocsismate kocsismate Awaiting requested review from kocsismate kocsismate is a code owner

@Girgias Girgias Awaiting requested review from Girgias Girgias is a code owner

Assignees
No one assigned
Labels
Extension: spl
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SplPriorityQueue does not support serialization
2 participants
@alexandre-daubois @TimWolla