fix: exception-safe engine-state restoration (fake scope, pDestructor, class-table guards) - #197
Merged
lisachenko merged 4 commits intoAug 13, 2026
Conversation
The property hooks paired setFakeScope() calls by hand around the original engine handler. Any throw from that handler - a throwing __get(), a typed property error, an uninitialized readonly access - skipped the restoring call and left EG(fake_scope) pointing at a foreign class entry for the rest of the request, silently changing visibility checks everywhere. Executor::withFakeScope() now owns the install/restore pair and restores in a finally block; all seven affected hooks (read/write/has/unset property, get property pointer, get properties for, get debug info) go through it, so the unsafe hand-rolled pattern can no longer be written. The baselined argument.type entries for the hook call sites are renamed to the new method - same pre-existing CData typing gap, new callee name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
Three sites hand-rolled the "null out pDestructor, delete, put it back" dance that HashTable::deleteWithoutDestructor() already performs safely: - ReflectionMethod::fromHookCData() did it inside its finally block WITHOUT a try, so a failing delete() left the publication board's function table with a NULL destructor for the rest of the process - every later bucket removal would then leak. The now-unused getRawValue() handle is dropped too. - HotSwap::unpublishClassEntry() and Core::shutdown() duplicated the same logic; ClassDelta already used the helper, so this just finishes the job. Semantics are unchanged: the destructor is still disabled for the delete, so the payload (a shared zend_function, a rehomed class entry, a function embedded in an immortalized closure) survives the bucket removal. Core::shutdown() keeps its find() pre-check - zend_hash_del() reports an absent key as FAILURE, which HashTable::delete() turns into an exception. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
HashTable::find() returns null for an absent key, and addInterfaces() called
getRawClass() on the result unchecked. interface_exists() is not a sufficient
pre-check: it accepts a leading-backslash name ("\Countable") that strtolower()
never turns into a class-table key, so the lookup misses and the method dies
with a call-on-null instead of a ReflectionException - halfway through, with a
freshly allocated interface buffer already tracked.
Adds the same guard every sibling lookup carries (setParent(), ObjectEntry,
ClosureEntry, ...), which also retires a baseline entry.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
initLowLevelStructures() cached a HashTable wrapper over zobj->properties once, but setDynamicPropertiesPointer() rewrites that very field without touching the shadow copy. The only reader, __debugInfo(), therefore dumped a table the object no longer owns - possibly one already freed, i.e. a dangling read from a debug dump. Objects whose table was built after the entry (the common lazy case) were also missing from the dump entirely. The field is removed and the table resolved from the live pointer at dump time, so ObjectEntry keeps exactly one source of truth for zobj->properties. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
This was referenced Aug 13, 2026
Merged
lisachenko
marked this pull request as ready for review
August 13, 2026 19:45
lisachenko
deleted the
claude/php-repo-modernization-6pfu7y-exception-safety
branch
August 13, 2026 19:47
This was referenced Aug 13, 2026
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.
Four related fixes where engine-global state could be left corrupted if an operation threw halfway through, plus one stale-cache read.
1.
EG(fake_scope)is never restored when the original handler throwsSeven property hooks paired
setFakeScope()calls by hand:Any throw from the original handler — a throwing
__get(), a typed-property error, an uninitialized readonly access — skips the restoring call and leavesEG(fake_scope)pointing at a foreign class entry for the rest of the request, silently changing every visibility check that follows.Executor::withFakeScope(?CData $scope, Closure $body)(src/System/Executor.php) now owns the install/restore pair and restores in afinally. All seven call sites go through it, so the unsafe pattern cannot be written again:src/ClassExtension/Hook/ReadPropertyHook.phpsrc/ClassExtension/Hook/WritePropertyHook.phpsrc/ClassExtension/Hook/HasPropertyHook.phpsrc/ClassExtension/Hook/UnsetPropertyHook.phpsrc/ClassExtension/Hook/GetPropertyPointerHook.phpsrc/ClassExtension/Hook/GetPropertiesForHook.phpsrc/ClassExtension/Hook/GetDebugInfoHook.phpThe baselined
argument.typeentries for those call sites are renamed to the new callee — same pre-existingCDatatyping gap, no new baseline entries.2. Hand-rolled
pDestructordisable/restore duplicatingHashTable::deleteWithoutDestructor()Three sites reimplemented the "null out
pDestructor, delete, put it back" dance thatHashTable::deleteWithoutDestructor()(src/Type/HashTable.php:299) already performs with afinally:src/Reflection/ReflectionMethod.php(fromHookCData) did it inside itsfinallyblock with notry, so a failingdelete()left the publication board's function table with aNULLdestructor for the rest of the process — every later bucket removal there would leak. The now-unused$boardTable = $methodTable->getRawValue()handle is dropped with it.src/HotSwap/HotSwap.php(unpublishClassEntry) duplicated the logic;ClassDeltaalready used the helper, so this just follows the existing precedent.src/Core.php(shutdown()) duplicated it around the generated-function unpublish loop.Semantics are unchanged — the destructor is still disabled for the delete, so the payload (a shared
zend_function, a rehomed class entry, a function embedded in an immortalized closure) survives the bucket removal. Thefind() !== nullpre-check inCore::shutdown()is kept:zend_hash_del()reports an absent key asFAILURE, whichHashTable::delete()turns into aRuntimeException, so the guard is load-bearing.3. Missing null guard in
ReflectionClass::addInterfaces()src/Reflection/ReflectionClass.phpcalledgetRawClass()on the result ofHashTable::find()without checking fornull. Theinterface_exists()pre-check is not sufficient: it accepts a leading-backslash name ("\Countable") thatstrtolower()never turns into a class-table key, so the lookup misses and the method dies with a call-on-null instead of aReflectionException— halfway through, with a freshly allocated interface buffer already tracked.Adds the same guard every sibling lookup carries (
setParent(),ObjectEntry,ClosureEntry, …). This retires one baseline entry, removed by hand fromphpstan-baseline.neon.4. Stale shadow property in
ObjectEntrysrc/Type/ObjectEntry.phpcached aHashTablewrapper overzobj->propertiesonce ininitLowLevelStructures(), butsetDynamicPropertiesPointer()rewrites that very field without touching the shadow copy. The only reader,__debugInfo(), therefore dumped a table the object no longer owns — possibly one already freed, i.e. a dangling read from a debug dump. Objects whose table was built after the entry (the common lazy case) were missing from the dump entirely.The field is removed and the table resolved from the live pointer at dump time, so
ObjectEntrykeeps exactly one source of truth forzobj->properties.Validation
Local test execution was impossible in this container: it runs PHP 8.5.9 while this branch targets PHP 8.4. Per AGENTS.md's non-negotiable version-matching rule, nothing that reaches
Core::init()may be executed against a mismatched minor, so no test — and no code touching engine memory — was run here. CI must provide the test signal for this PR.Validation performed was static only, and both gates are clean:
composer phpstan(level max) — no errorscomposer cs:check(php-cs-fixer,@PER-CS2.0) — no fixable filesNo generated artifacts (
include/,stubs/,.phpstorm.meta.php) were touched.🤖 Generated with Claude Code
https://claude.ai/code/session_01RnoZ7wuGepCTzsmFQ5sKxG
Generated by Claude Code