fix(core): make CastObjectHook fall through to default engine behaviour safely - #156
Merged
Conversation
…ur safely Two defects made the documented fall-through pattern - proceed() then getResult() - unusable for cast types the default handler cannot satisfy: - The engine hands cast_object an UNINITIALIZED retval slot, and zend_std_cast_object_tostring returns FAILURE for numeric casts without writing it (the engine caller is the one that warns and substitutes 1). getResult() dereferenced that slot regardless, running the by-ref ReferenceEntry machinery over garbage - which observably unsets local variables in the calling frames and emits spurious "Undefined variable" warnings. - handle() unconditionally reported Core::SUCCESS, so a FAILURE from the original handler could never propagate: whatever the user handler returned - including an accidental null - was silently installed as the cast result, and the engine caller's default behaviour (warning plus substitute value for numeric casts, engine Error for string casts) was unreachable. proceed() now records its status. getResult() refuses to read the slot unless the last proceed() succeeded and returns null instead, and handle() propagates FAILURE to the engine when the user handler fell through (returned null after a failed proceed). Together this makes the naive fall-through behave exactly like an uninstalled handler for every cast type; the regression test asserts the engine's own diagnostic and substitute value come back and that no corruption noise is emitted. Fixes #153 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JsbdqisRfGuujN3QD9n8En
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.
Fixes #153.
The bugs
Two defects made the documented fall-through pattern —
$hook->proceed(); return $hook->getResult();— unusable for cast types the default handler cannot satisfy:getResult()read uninitialized memory. The engine handscast_objectan uninitialized retval slot, andzend_std_cast_object_tostringreturnsFAILUREfor numeric casts without writing it (the engine caller is what warns and substitutes1).getResult()dereferenced the slot regardless, running the by-refReferenceEntrymachinery over garbage — observably unsetting local variables in the calling frames ($resultinhandle()itself became undefined after being assigned) and emitting spurious "Undefined variable" warnings.handle()always reportedCore::SUCCESS, so aFAILUREfrom the original handler could never propagate. Whatever the user handler returned — including an accidentalnull— was silently installed as the cast result, and the engine caller's default behaviour (warning + substitute value for numeric casts, engineErrorfor string casts) was unreachable.The fix
proceed()records its status.getResult()refuses to read the slot unless the lastproceed()succeeded, returningnullinstead of dereferencing scratch memory.handle()propagatesFAILUREto the engine when the user handler fell through (returnednullafter a failedproceed()). The failure is surfaced by the engine's own caller after the FFI callback has returned, so even the throwing default paths (string casts) stay safe.Together this makes the naive fall-through behave exactly like an uninstalled handler for every cast type.
The regression test
testCastObjectHandlerFallsThroughToEngineDefaultinstalls the naive fall-through handler and asserts:true(viagetResult()after a successful proceed);(int)yields1with the engine's own diagnosticObject of class ... could not be converted to intand — by capturing every PHP diagnostic into a strictassertSame— no "Undefined variable" corruption noise.Against the unfixed hook the test fails with exactly the reported symptom (captured warning is
Undefined variable $resultinstead of the engine diagnostic); with the fix it passes.Verified: full suite green on PHP 8.4.19 NTS (409 tests), PHPStan level max clean, PER-CS2.0 clean.
Note: this PR touches the same file as #155 in non-overlapping regions; both are independent of each other and either merge order works.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JsbdqisRfGuujN3QD9n8En
Generated by Claude Code