Skip to content

Commit e8e7544

Browse files
ondrejmirtesclaude
andcommitted
Keep a node's subnode array alive while the native traverser walks it
traverseNode() separated the subnode array so it could be mutated in place and then handed traverseArray() the property's own table - owned by nothing but that property. traverseArray() walks the buckets directly, and between two of them it calls the visitor hooks, which run arbitrary PHP. That code can reach the very property being walked: PHPStan resolves a PHPDoc from inside ExportedNodeVisitor::enterNode(), which re-enters the parser for the file currently being traversed. Assigning to the property, or dropping the node's last reference, then frees the table under the loop, and a write into it reallocates the buckets the iterator holds - the traversal reads freed memory and reports "Invalid node structure: Contains nested arrays", or segfaults. The PHP twin never sees this: `$subNode = $node->$name` gives traverseArray() a local of its own, so the table cannot go away and any writer separates first. The native side now holds the same reference for the duration of the call. It costs one addref and one release per subnode array, no duplication: self-analysis under the extension measured 17.97s against 18.73s for the build without it, so the difference is noise. Reproducer, both files fetched in one process with the extension loaded: ExportedNodeFetcher::fetchNodes() on tests/PHPStan/Analyser/data/bug-7094.php and then on tests/PHPStan/Analyser/nsrt/bug-3142.php. Over all 5129 files under tests/*/data and tests/PHPStan/Analyser/nsrt, 8 threw before this change and none do after; the PHP twin never threw on any of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WvMWsNHbCdSgx97Y56XYcU
1 parent 3eb12b6 commit e8e7544

1 file changed

Lines changed: 16 additions & 0 deletions

File tree

‎turbo-ext/src/NodeTraverser.cpp‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,22 @@ class NodeTraverser
360360
if (value.isArray()) {
361361
/* separate so we can mutate in place */
362362
SEPARATE_ARRAY(value.raw());
363+
/* Hold a reference to the table for the whole traversal, the
364+
* way the PHP twin's `$subNode = $node->$name` local does.
365+
* traverseArray() walks the buckets in place while the visitor
366+
* hooks in between run arbitrary PHP, and that code can reach
367+
* this very property: PHPStan resolves a PHPDoc from inside
368+
* enterNode(), which re-enters the parser for the file being
369+
* traversed. With the array owned by the property alone,
370+
* assigning to the property - or dropping the node's last
371+
* reference - frees the table under the loop, and a write into
372+
* it reallocates the buckets the iterator holds; the traversal
373+
* then reads freed memory, which surfaces as "Invalid node
374+
* structure: Contains nested arrays" or a segfault. A reference
375+
* of our own keeps the table alive, and being shared it makes
376+
* any writer separate first, so the iteration always sees the
377+
* array it started with. */
378+
zv::Val arrayGuard = zv::Val::copyOf(zv::Ref(value.raw()));
363379
zv::Arr replacement = traverseArray(zv::ArrRef(value.raw()));
364380
if (UNEXPECTED(failed)) {
365381
return;

0 commit comments

Comments
 (0)