Repository navigation
Fix stale PFA cache entries after the declaring site is freed - #25374
Conversation
The request PFA cache is keyed on the address of the declaring site. When eval()'d code or a class constant AST was freed, a new PFA could reuse that address and get the cached function for a different call.
| /* The cached PFA keeps a non-interned name alive, unlike a non-persistent AST */ | ||
| const uint32_t *cache_key = ZSTR_IS_INTERNED(fcc_ast->name) | ||
| ? &ast->lineno : (const uint32_t *) fcc_ast->name; |
There was a problem hiding this comment.
There could be edge cases where "interned" doesn't imply "persistent AST" and vis versa, for example because the opcache interned string buffer is full, or the string is interned but opcache is disabled.
Can we use flags & ZEND_PARTIAL_CACHEABLE_IN_SHM instead?
Another issue is that names are not guaranteed unique. For the non-SHM case, the address of name will be unique anyway because we don't intern those until persistence, but this is fragile. I would suggest to add a comment in zend_compile_const_expr_fcc() next to the zend_compile_partial_name() call, and an assertion here (ZEND_ASSERT(!ZSTR_IS_INTERNED(fcc_ast->name) || (flags & ZEND_PARTIAL_CACHEABLE_IN_SHM))).
There was a problem hiding this comment.
Switched to flags & ZEND_PARTIAL_CACHEABLE_IN_SHM, thanks. I also added tests covering edge cases you mentioned about.
| /* function_add_ref() ignores ZEND_EVAL_CODE */ | ||
| copy->type = ZEND_USER_FUNCTION; |
There was a problem hiding this comment.
Can we avoid adding op_array to partial_function_application_cache for eval'ed functions? Then we can avoid retaining the declaring op_array if it's an eval'ed function.
There was a problem hiding this comment.
I tried it, but it's more complex than retention. Without the cache entry, the closure is the only owner of the op_array, so the runtime cache slot can't be used either. That requires eval-specific handling in both opcache and zend_partial_create(), and PFAs in loops in eval'd code are recompiled on every iteration. Edge case: 100k iterations went from ~0.6s to ~5.6s and +27MB.
Retention costs one entry per eval execution, which the cache already did before this fix, and it also covers included files without opcache. I don't know perfect solution for this problem.
Interned names don't imply SHM ASTs (e.g. file_cache_only), and names are not unique by content.
* PHP-8.6: [ci skip] Updated NEWS Bump API version numbers
* PHP-8.6: Fix error buffer cleanup in phar_open_parsed_phar() (php#25376) Fix phpGH-25306: Assertion status == LXB_STATUS_OK in dom_parse_selector when lexbor allocation fails Fix AST printing dropping readonly on promoted parameters (phpGH-25207) Fix truncated anonymous class names in error messages and logs (phpGH-25352) ext/gd: blame the points array when a polygon has too few points ext/gd: keep the brush and tile images alive while the destination uses them ext/standard/dns.c: drop partially built records in php_parserr()
* PHP-8.6: Fix stale PFA cache entries after the declaring site is freed (#25374)
|
Thank you! |
The request PFA cache is keyed on the address of the declaring site (
&opline->linenoor&ast->lineno). Without SHM caching, this memory can be freed while the cache entry is still alive:A later PFA allocated at the same address then hits the stale entry and gets an op_array compiled for a different argument shape. This leads to wrong bound arguments, or out-of-bounds writes in
zp_bind()when the bound var counts differ.Fix:
dynamic_func_defs, like it already does for called closures.function_name. Interned names mean the AST is in persistent memory, so the address key is kept.Trade-off: PFAs in eval()'d code keep that op_array alive until the end of the request.