Fix GH-19653: Closure named argument unpacking between temporary closures can cause a crash #19654
+37
−4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…losures can cause a crash Due to closures, the `fbc` address isn't unique if the memory address is reused. So for user closures we need to distinguish using a unique key, while internal functions can't disappear and therefore can use the `fbc` address as unique key. As for performance: This adds a load+compare for the function type, although we already load and compare the function type down below, so that impact on performance should be small. It also adds a load on the opcodes array, but we access fields from the same cacheline too so that should also have only a small impact.
dstogov
approved these changes
Sep 1, 2025
There was a problem hiding this comment.
For some reason, I didn't understand the problem from the explanation at the first place, but when I started writing a comment, I found that the explanation says almost the same that I liked to say :) - "fbc" can't be stored in cache, because PHP may free the closure and allocate another one (with the same address).
arnaud-lb
requested changes
Sep 1, 2025
Comment on lines
+5061
to
+5064
| /* Due to closures, the `fbc` address isn't unique if the memory address is reused. | ||
| * So for user closures we need to distinguish using a unique key, while internal functions can't disappear | ||
| * and therefore can use the `fbc` address as unique key. */ | ||
| void *unique_id = EXPECTED(fbc->type == ZEND_USER_FUNCTION) ? (void *) fbc->op_array.opcodes : (void *) fbc; |
There was a problem hiding this comment.
fbc->op_array.opcodes is not unique unless ZEND_ACC_IMMUTABLE is set. E.g.
function usage1(Closure $c) {
$c(a: 1);
}
usage1(eval('return function($a) { var_dump($a); };'));
usage1(eval('return function($b) { var_dump($b); };'));
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Due to closures, the
fbcaddress isn't unique if the memory address is reused. So for user closures we need to distinguish using a unique key, while internal functions can't disappear and therefore can use thefbcaddress as unique key.As for performance:
This adds a load+compare for the function type, although we already load and compare the function type down below, so that impact on performance should be small. It also adds a load on the opcodes array, but we access fields from the same cacheline too so that should also have only a small impact.
It can make a performance improvement for shared opcodes arrays in case of traits as the cache slot can be reused.