Skip to content

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
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

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

alexandre-daubois commented Aug 12, 2025

I addressed all your comments in 3f041a3

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
@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.

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.

@alexandre-daubois
Copy link
Member Author

Thanks for the extensive review @TimWolla!

@Girgias Girgias removed their request for review September 8, 2025 12:46
@Girgias
Copy link
Member

Girgias commented Sep 8, 2025

I don't really know how SplHeap works, maybe @arnaud-lb or @iluuu1994 can review this instead?

@arnaud-lb
Copy link
Member

@TimWolla

Should we prevent serialization if the SPL_HEAP_CORRUPTED flag is set?

Looking at the code, this doesn't seem to be necessary. The flag is only added when order is not guaranteed anymore. From an engine/memory perspective, the heap is still consistent. The heap remains readable, and the flag can be reset by userland (SplHeap::recoverFromCorruption()). Therefore it seems safe to allow serialization in this case.

@alexandre-daubois
Copy link
Member Author

Thanks Arnaud! Comments addressed

@arnaud-lb
Copy link
Member

arnaud-lb commented Sep 10, 2025

@TimWolla

Should we prevent serialization if the SPL_HEAP_CORRUPTED flag is set?

Looking at the code, this doesn't seem to be necessary. The flag is only added when order is not guaranteed anymore. From an engine/memory perspective, the heap is still consistent. The heap remains readable, and the flag can be reset by userland (SplHeap::recoverFromCorruption()). Therefore it seems safe to allow serialization in this case.

Actually, trying to unserialize a corrupted heap would likely throw, and fail, so it's not useful to allow serialization of a corrupted heap. The PR currently prevents it, too.

Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

This looks good to me!

I don't think it qualifies as a bugfix, so merging in master right now would require @php/release-managers-85 approval.

@Girgias Girgias requested a review from a team September 11, 2025 09:45
Copy link
Member

@DanielEScherzer DanielEScherzer left a comment

Choose a reason for hiding this comment

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

RM approval, technical review not performed

@alexandre-daubois alexandre-daubois merged commit e6c07b0 into php:master Sep 22, 2025
9 checks passed
@alexandre-daubois alexandre-daubois deleted the spl-heap-serialize branch September 22, 2025 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SplPriorityQueue does not support serialization

5 participants