Skip to content

standard: Fail unserialization when the C format is used for classes that are not Serializable#22058

Merged
kocsismate merged 4 commits into
php:masterfrom
kocsismate:gh22046-fix
Jun 12, 2026
Merged

standard: Fail unserialization when the C format is used for classes that are not Serializable#22058
kocsismate merged 4 commits into
php:masterfrom
kocsismate:gh22046-fix

Conversation

@kocsismate

@kocsismate kocsismate commented May 15, 2026

Copy link
Copy Markdown
Member

Unserializing from the "C" format is explicitly disabled.

Fixes GH-22046.

@kocsismate kocsismate requested a review from TimWolla as a code owner May 15, 2026 21:32
@kocsismate kocsismate changed the base branch from master to PHP-8.5 May 15, 2026 21:32

@TimWolla TimWolla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As mentioned in the issue, this should get a general fix. But the patch LGTM if we decide for some reason not to fix this generally.

@iluuu1994 iluuu1994 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO, fuzzing-style issues (things that are effectively impossible to happen by accident) should be fixed on master, assuming they don't pose a security risk (which this does not).

Comment thread ext/standard/var_unserializer.re Outdated
Comment thread ext/standard/var_unserializer.re Outdated
@kocsismate

Copy link
Copy Markdown
Member Author

MO, fuzzing-style issues (things that are effectively impossible to happen by accident) should be fixed on master, assuming they don't pose a security risk (which this does not).

Yes, I think you are right! So I changed the target branch to master.

@TimWolla TimWolla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

PR should be retitled to indicate that it's not a fix that is specific to ext/uri. UPGRADING and NEWS should be added. But the change itself LGTM.

Comment thread ext/standard/var_unserializer.re

@TimWolla TimWolla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've made an(other) (opinionated) retitle of the PR to describe the change that has been made, rather than describing the symptoms.

Diff LGTM now. Don't forget to add NEWS.

[skip ci]
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.

The unserialize function can lead to segfault when non-Serializable internal classes are serialized back with the C format

4 participants