fix(type): read opline operands off the znode_op union instead of casting it - #227
Merged
Merged
Conversation
…ting it
OpLine::getValuePointer() reached the operand through Core::cast('znode_op *',
$node). $node IS the znode_op union already, so that asks FFI to reinterpret a
4-byte union VALUE as an 8-byte pointer, and FFI answers "attempt to cast to
larger type" - for every IS_CV/IS_VAR/IS_TMP_VAR operand and, through
getRuntimeConstant(), for every runtime constant. Reading an opline operand
from a user opcode handler has been impossible since the struct-stub migration
(#190). The fields are now read straight off the union.
Consumers meet that failure as silence rather than as an error, which is what
made it expensive to find: a user opcode handler runs inside an FFI callback,
where an escaping throw is a fatal engine abort, so every serious consumer
wraps its handler in a catch-all. The exception disappears there and the
symptom is an instruction that never seems to arrive. Downstream in ZDebug it
looked like exception breakpoints and return-value debugging had silently
stopped firing, with nothing in any log.
The rest of the change is what sharper types then exposed. The redundant
'zend_op *' cast on an already-typed opline is gone, the two operand params
carry their generated stub views, and the zval cast uses the stub-class form
that types the handle for analysis. Two baseline entries went with it: the
'mixed' they recorded came from the casts this removes.
Covered by two tests on OpCodeHookTest - reading a compiled-variable operand
and reading a constant one - both of which fail on master with the cast error.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JCNB8eDgS6NEnfCFw7tPjU
lisachenko
force-pushed
the
claude/fix-opline-operand-and-hook-recursion
branch
from
August 16, 2026 20:21
00dcdd4 to
e640f53
Compare
Reading the operand off the union types $constantOffset as int, which turns
OpLine's pointer arithmetic into the same "Binary operation + between FFI\CData
and int" that Compiler.php and ExecutionData.php already carry for the identical
Core::cast('char *', ...) + int construct. Recorded the same way they are, and
the "and mixed" entry it replaces is dropped.
Not verifiable outside CI: the analyser available in this environment infers
Core::cast() differently and reports neither message, so CI's PHPStan leg is the
only thing that can confirm the entry matches.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JCNB8eDgS6NEnfCFw7tPjU
Owner
Author
|
What about php8.4 branch - do we need to backport a fix there as well? |
6 tasks
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
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.
Reading an opline operand from a user opcode handler has been impossible since the struct-stub migration (#190). Found by ZDebug, whose exception breakpoints and return-value debugging both went silently dead against current
master.The bug
OpLine::getValuePointer()reached the operand through:$nodeis theznode_opunion already. Casting it toznode_op *asks FFI to reinterpret a 4-byte union value as an 8-byte pointer, and FFI answers:for every
IS_CV/IS_VAR/IS_TMP_VARoperand, and the same ingetRuntimeConstant()for every runtime constant. The fields are now read straight off the union.Why it presents as silence. A user opcode handler runs inside an FFI callback, where an escaping throw is a fatal engine abort — so every serious consumer wraps its handler in a catch-all. The exception disappears there, and the symptom is an instruction that never seems to arrive. Downstream this looked like
THROWandRETURNhooks that had simply stopped firing, with nothing in any log.The rest of the diff
Only what sharper types then exposed:
Core::cast('zend_op *', …)on an already-typed opline is gone;znode_op), so the field reads type asint;Core::cast(zval::class, …), which types the handle for analysis;phpstan-baseline.neonentries are removed — themixedthey recorded came from the casts this deletes.Verification
Two new tests on
OpCodeHookTest, both of which fail onmaster:testHandlerCanReadCompiledVariableOperandsFFI\Exception: attempt to cast to larger typetestHandlerCanReadConstantOperandsDownstream, ZDebug's suite goes from 4 failures to green against this branch: 291 unit + 41 integration, 0 failures, including the paths that read operands from inside
THROWandRETURNhandlers.php-cs-fixer --dry-runclean; PHPStan clean forsrc/(thetests/leg needsphpstan-phpunit, which is not installable in my environment — CI covers it).Separately: a recursion hazard I could not pin down
While investigating I hit a second, unrelated failure — hooking
RETURNin a cold process stack-overflowed during install:The mechanism looks clear —
handle()'s scope check reachesCore::engineConstant(), which lazilyrequires the constants artifact; that require's own top-levelRETURNre-enters a guard that cannot recognise a scope-less frame as z-engine's, andCore::$engineConstantsis only assigned once the require completes.I have deliberately left the fix out of this PR. I could not build a test that reproduces it — every scenario I wrote boots through
vendor/autoload.php, which warms the artifact before a hook can be installed — and I am not willing to ship a change toinstall()I cannot demonstrate. Recording it here so it is not lost; happy to pursue it separately if you can suggest a reliably cold entry point.