Skip to content

Fix GH-21831: Defer removals during SplObjectStorage::removeAllExcept()#21835

Open
prateekbhujel wants to merge 2 commits intophp:masterfrom
prateekbhujel:prateekbhujel/fix-gh-21831-removeallexcept-reentry
Open

Fix GH-21831: Defer removals during SplObjectStorage::removeAllExcept()#21835
prateekbhujel wants to merge 2 commits intophp:masterfrom
prateekbhujel:prateekbhujel/fix-gh-21831-removeallexcept-reentry

Conversation

@prateekbhujel
Copy link
Copy Markdown
Contributor

Fixes GH-21831.

SplObjectStorage::removeAllExcept() may detach from the storage while a custom getHash() implementation is still able to re-enter and mutate that same storage.

Defer those removals until after the scan finishes so the iteration stays stable.

Copy link
Copy Markdown
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

I'm not sure I like this approach, but not sure how what I would want the behaviour to be is achievable.

Ideally one shouldn't be able to mess with the state from the getHash() method, as it's meant to be somewhat of a pure function.

@prateekbhujel
Copy link
Copy Markdown
Contributor Author

Yeah, I agree with that.

getHash() really should behave like a pure lookup, so being able to mutate either storage from there is pretty awkward. I kept this one narrow and just made removeAllExcept() resilient to that re-entrancy, rather than trying to change what getHash() is allowed to do as part of a bug fix.

@Girgias
Copy link
Copy Markdown
Member

Girgias commented Apr 23, 2026

I think it would be cleaner to just change what getHash() is allowed to do, considering you are already targeting master and this seems very artificial so not sure it's worth backporting

@prateekbhujel
Copy link
Copy Markdown
Contributor Author

I tried that direction locally.

The tricky part is that master already includes the recent concurrent-deletion coverage added in 43a4f91 (GH-21443), and once getHash() starts rejecting mutation those cases begin to fail. So that turns it into a behavior change, not just a cleanup around this UAF.

That’s why I kept this one narrow: fix the use-after-free in removeAllExcept() without changing the current getHash() re-entrancy behavior.

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.

Uaf in SplObjectStorage::removeAllExcept() with custom getHash()

2 participants