Fix GH-22857: JIT wrong code for FETCH_OBJ_FUNC_ARG with property hooks - #22897
Fix GH-22857: JIT wrong code for FETCH_OBJ_FUNC_ARG with property hooks#22897zhaohao19941221 wants to merge 5 commits into
Conversation
…hooks FETCH_OBJ_FUNC_ARG dispatches into the FETCH_OBJ_R handler for by-value argument fetches (see ZEND_FETCH_OBJ_FUNC_ARG in zend_vm_def.h). When the shared property cache slot has the SIMPLE_GET bit set (either primed by the same FETCH_OBJ_FUNC_ARG opline on a previous iteration, or by a sibling FETCH_OBJ_R on the same property whose cache slot is merged by compact_literals), the FETCH_OBJ_R handler takes the SIMPLE_GET fast path and pushes the getter call frame, returning opline | ZEND_VM_ENTER_BIT to signal the caller to re-enter the VM. Function JIT handles this only for FETCH_OBJ_R via a hook-enter guard that compares the returned IP against opline+1 and tail-calls into the new frame when they differ. FETCH_OBJ_FUNC_ARG was compiled through the generic default handler with no such guard, so the JIT-emitted SEND_FUNC_ARG that follows read the argument slot before the getter had actually run. Depending on the adjacent property slot's contents, the symptom is a spurious 'must not contain any null bytes' ValueError, a 'expected string, got <adjacent-class>' TypeError, or a zend_mm_heap corruption abort. Compile FETCH_OBJ_FUNC_ARG through the same path as FETCH_OBJ_R so it emits the hook-enter guard. Because FETCH_OBJ_FUNC_ARG is not handled by the INLINE-only switch above, the shared 'ce' local can carry a stale value from a previous opline iteration; reset it to NULL for FETCH_OBJ_FUNC_ARG so the guard is always emitted regardless of any final/no-hooks fast path that the FETCH_OBJ_R case may otherwise skip. The by-reference dispatch (to FETCH_OBJ_W) never takes the SIMPLE_GET fast path, so the guard is a no-op there. This mirrors the JIT-only shape used for the tracing JIT in phpGH-21369 (which fixed the analogous phpGH-21006 for tracing). Fixes phpGH-22857
|
Ping @arnaud-lb @iliaal — this is the JIT-only rework you suggested; would appreciate another look when you have time. Thanks! |
…p read The sibling-slot variant used '$warm = $this->path;' to prime the SIMPLE_GET bit. Storing the hook result in a local variable trips a pre-existing, unrelated function JIT memory leak that also reproduces on master without this fix (hooked FETCH_OBJ_R result assigned to a CV leaks one string per call at opcache.jit=1203+). Transfer the hook result into a property instead, which still primes the shared cache slot but keeps this test independent of that separate bug. The FETCH_OBJ_FUNC_ARG coverage is unchanged: without the JIT fix this test still crashes, and with it it passes leak-free.
There was a problem hiding this comment.
I don't see the reason to loop it 200 times. In your original reproduce script you loop it 5000 times
If I am understanding your bug correctly, two or three calls can reproduce the bug. But maybe I am missing something
Update: I test this locally, with only one single $c->step() call can stably reproduce the bug.
Co-authored-by: Weilin Du <weilindu@php.net>
Co-authored-by: Weilin Du <weilindu@php.net>
@LamentXU123 Thanks for the review — you're right that the bug triggers after just a couple of calls, so let me clarify why the loop is there, since it's not about the number of calls needed to trigger it. Mechanism: on the first step() the function runs in the VM and primes the SIMPLE_GET bit on the property's cache slot (slow path, getter runs correctly). The second call is JIT-compiled and consumes that primed bit; without the guard the JIT reads whatever sits in the adjacent property slot instead of running the getter. So ~2 iterations is enough to trigger the wrong behavior. The 200-iteration loop is a reliability measure for the regression test, not a trigger requirement. The unguarded read consumes whatever bytes happen to be in the sibling slot, and whether that produces a visible failure (a thrown \Error/\TypeError from file_get_contents, or heap corruption) is data-dependent and non-deterministic. With only 2–3 iterations a buggy build can occasionally slip through (false negative), defeating the purpose of a regression test — I verified in the PR description that without the fix it fails 20/20, and the loop is what makes that consistent. This matters most for the second variant (Container2): compact_literals can merge the cache slots of the preceding FETCH_OBJ_R and the FETCH_OBJ_FUNC_ARG, so whether the shared-slot case is actually hit is sensitive to literal compaction / JIT recompilation timing — a loop guarantees we exercise it. If you'd prefer a smaller loop I'm happy to shrink it, or alternatively I can make the assertion deterministic by checking the property value directly instead of relying on file_get_contents fataling — that would let us shrink the loop while still reliably failing on a buggy build. Let me know which you prefer. |
|
for Container2, the loop itself does not make compact_literals merge the cache slots. that should be determined at compile time. The loop only repeats the bad read once that shape is already present. I would prefer the deterministic assertion approach you mentioned. IMO A regression test that needs 200 iterations to become reliable is usually a sign that we should assert the wrong value behavior more directly. |
Summary
Fix GH-22857: heap corruption / spurious
ValueError/TypeErrorwhen a property hook is read viaFETCH_OBJ_FUNC_ARGunder function JIT (opcache.jit=1205).This supersedes the earlier engine-side attempt in the previous PR (
fix-22857-JIT-virtual-property-hook-as-arg-to-unqualified-namespaced-fallback-coderzhao-2026-07-22). Based on review feedback from @arnaud-lb and @iliaal, the fix has been rewritten as a JIT-only change that mirrors the shape of GH-21369 (which fixed the analogous GH-21006 for tracing JIT).Fixes GH-22857.
Root cause
ZEND_FETCH_OBJ_FUNC_ARGdispatches into theZEND_FETCH_OBJ_Rhandler for by-value argument fetches (seezend_vm_def.h). When the shared property cache slot has theSIMPLE_GETbit set, theFETCH_OBJ_Rhandler:EG(current_execute_data)to the new frame.opline | ZEND_VM_ENTER_BITto signal the caller to re-enter the VM at the new frame.The
SIMPLE_GETbit can be set in two ways:FETCH_OBJ_FUNC_ARGopline primed it on a previous iteration (via slow path inzend_std_read_property()).FETCH_OBJ_Ron the same property primed it, andcompact_literalshas merged the two oplines' cache slots.Function JIT already handles this case for
ZEND_FETCH_OBJ_Rvia a hook-enter guard inzend_jit.cthat compares the returned IP againstopline+1and tail-calls into the new frame when they differ. However,ZEND_FETCH_OBJ_FUNC_ARGwas compiled through the genericdefault:branch with no such guard, so the JIT-emittedSEND_FUNC_ARGthat follows read the argument slot before the getter had actually run.Depending on the adjacent property slot's contents, the symptom is:
ValueError: file_get_contents(): Argument #1 ($filename) must not contain any null bytes(deterministic 20/20 in the reproducer)TypeError: <fn>(): Argument #N ($arg) must be of type ?string, <adjacent-class> given(observed in the real-world Hyperf/Swoole application that led to this report)zend_mm_heap corrupted→SIGABRT(also observed in the real-world code)All three symptoms have the same root cause:
SEND_FUNC_ARGpicking up garbage from an adjacent property slot before the getter has run.Fix
Compile
ZEND_FETCH_OBJ_FUNC_ARGthrough the same path asZEND_FETCH_OBJ_Rso it emits the hook-enter guard. BecauseZEND_FETCH_OBJ_FUNC_ARGis not handled by the INLINE-only switch above, the sharedcelocal can carry a stale value from a previous opline iteration; reset it toNULLforFETCH_OBJ_FUNC_ARGso the guard is always emitted regardless of anyfinal/no-hooks fast path that theFETCH_OBJ_Rcase may otherwise skip.The by-reference dispatch (to
FETCH_OBJ_W) never takes theSIMPLE_GETfast path (that fast path only exists in theFETCH_OBJ_Rhandler), so the guard is a run-time no-op there: the by-ref path always returns withIP == opline+1and theIR_IF_TRUEbranch is taken.This mirrors the JIT-only shape used for the tracing JIT in GH-21369 (which fixed the analogous GH-21006 for tracing).
Alternatives considered
zend_std_read_property()(the shape of the previous PR): only primeSIMPLE_GETwhen the current opline isZEND_FETCH_OBJ_R. Rejected because it does not close the sibling-primed variant ($warm = $this->path; @file_get_contents($this->path);) that @iliaal called out —compact_literalsshares the cache slot, and the JIT-compiledFUNC_ARGstill consumes a bit primed by the siblingFETCH_OBJ_R.FETCH_OBJ_FUNC_ARGinto by-value / by-ref paths in JIT (@arnaud-lb's suggestion of dispatching tozend_jit_fetch_obj()for by-value and a cold-path handler for by-ref): would work but is a larger change; the current shape reuses the exact same generic-handler + hook-enter-guard idiom theFETCH_OBJ_Rcase already uses, keeping the fix minimal.Necessary conditions (established by systematic delta-debugging)
Removing any one of these makes the bug disappear. Each condition was verified with 20 runs per variant:
opcache.jit=1205(function JIT).disableandtracingare 20/20 OK.namespace.\, nouse function) soINIT_NS_FCALL_BY_NAME+FETCH_OBJ_FUNC_ARGis emitted (optimizer can't resolve namespaced-fallback targets, so theFETCH_OBJ_FUNC_ARGisn't rewritten toFETCH_OBJ_R).FETCH_OBJ_FUNC_ARG). Assigning to a local first usesFETCH_OBJ_Rand hides the bug.@(BEGIN_SILENCE/END_SILENCE).get =>calls a static method reading the promoted asymmetric properties.Reproducer (before the fix)
Without the fix:
20/20crashing / erroring with the symptoms above.With
opcache.jit=disableoropcache.jit=tracing:20/20clean.With the fix applied:
20/20clean for all three JIT modes.Test
ext/opcache/tests/jit/gh22857.phpt— 200 iterations covering both trigger paths:Container::step()— original issue: the sameFETCH_OBJ_FUNC_ARGopline primesSIMPLE_GETon iteration 1 and consumes it on iteration 2.Container2::step()— sibling-slot variant that @iliaal pointed out: a precedingFETCH_OBJ_Ron the same property primes the shared cache slot, and the followingFETCH_OBJ_FUNC_ARGconsumes it.Fails before the fix (20/20 under
jit=1205), passes after.Backport
The bug has been present since PHP 8.4 (property hooks introduced in 8.4). Targeting
master(PHP 8.6-dev) as suggested; happy to also targetPHP-8.4/PHP-8.5if maintainers prefer.Related
FETCH_OBJ_FUNC_ARGon a virtual property hook when calling an unqualified namespaced-fallback function #22857Thanks @arnaud-lb and @iliaal for the review feedback that redirected this to the correct fix.