Skip to content

Stop turbo-ext from touching refcounts of immutable arrays and interned strings - #6561

Merged
ondrejmirtes merged 5 commits into
2.3.xfrom
turbo-immutable-refcount-fixes
Sep 23, 2026
Merged

ondrejmirtes merged 5 commits into
2.3.xfrom
turbo-immutable-refcount-fixes

Conversation

@ondrejmirtes

Copy link
Copy Markdown
Member

Supersedes #6558. It keeps @zonuexe's commit and adds fixes for the other places in turbo-ext with the same class of bug.

In PHP, a shared read-only array or string must never have its refcount incremented. For arrays that means the shared empty array of a [] literal (it lives in .rodata), for strings any interned string, which with opcache lives in shared memory. ZVAL_ARR and Z_ADDREF_P still increment it, which writes into memory that may be read-only.

Commits

  1. Pass immutable array arguments non-refcounted through zv::Args
    • This is the commit from Pass immutable array arguments non-refcounted through zv::Args #6558 by @zonuexe. zv::Args wrapped a HashTable * with a bare ZVAL_ARR, and the addref in zend_call_function() then wrote into zend_empty_array.
    • The crash is reachable when ForeachHandler hands empty conditional holder tables to a PHP override of MutatingScope::addConditionalExpressions().
    • I amended the test to call setAccessible(true) before ReflectionProperty::getValue() (required on PHP < 8.1, where the old-PHPUnit jobs failed with Cannot access non-public member). I added the matching ignore to build/php-85.neon, like the one for ObjectTypeTest.
    • Verified by rebuilding the extension with only the zv.h change reverted: the array{} data set exits with 138 (SIGBUS) on macOS arm64. With the change it passes, and without the extension it passes either way.
  2. Do not addref interned parameter names in native PhpDocsResolver
    • positionalNames() did Z_ADDREF_P on each parameter's Variable name. The native parser allocates those strings, but names built by PHP code (e.g. a new Variable('foo') literal) are interned.
    • It is now Z_TRY_ADDREF_P.
    • The new PhpDocsResolverTest builds the method node in PHP. Before the fix it exits with 138 under -d opcache.enable_cli=1 -d opcache.protect_memory=1 with the extension loaded; after the fix it passes. CI doesn't run with protect_memory, so there the test only checks the resolved parameter types. It won't catch a regression of the refcount write itself.
  3. Share the per-slot key table through copyOfTable in VariableLivenessResolver
    • This replaces a bare ZVAL_ARR + Z_ADDREF on a borrowed table with zv::Arr::copyOfTable().
    • It can't crash today, because keysBySlot entries are always separated tables. Hardening only.
  4. Separate optional keys after taking the snapshot in makeOffsetRequired()
    • The "snapshot" shared the table that the loop deletes from, so the deletes went into a table with refcount 2 while it was being iterated.
    • The results were right, but it trips HT_ASSERT_RC1 on debug PHP builds. Taking the snapshot before separate() fixes it, still with one duplication.
  5. make bump-turbo

Audit

I checked every other ZVAL_ARR/RETURN_ARR, every refcount change on arrays, the raw array writes and every zv::Args use in turbo-ext/src. The other sites wrap freshly allocated tables, already mark shared constants as non-refcounted, or only read the table.

Closes #6558

🤖 Generated with Claude Code

https://claude.ai/code/session_01EKX5yrrUnnbeRtc2oURC2u

zonuexe and others added 5 commits September 23, 2026 12:24
zv::Args stored a HashTable argument with a bare ZVAL_ARR, which types
an immutable table (the shared zend_empty_array of a PHP [] literal or
zv::Arr::empty()) as refcounted. When the argument vector reached a PHP
method, the addref in zend_call_function() wrote into read-only memory
(SIGBUS on macOS arm64, SIGSEGV on Linux).

ForeachHandler hit this for a constant array without keys: its
conditional holder tables stay empty and are handed to a PHP override
of MutatingScope::addConditionalExpressions(). Wrap immutable tables as
plain IS_ARRAY, like zv::Arr::adoptTable() and copyOfTable() do.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
positionalNames() collected each parameter's Variable name with
Z_ADDREF_P, which increments the refcount of an interned string too.
Names built by PHP code (a `new Variable('foo')` literal) are interned,
and with opcache those live in shared memory: the write faults under
opcache.protect_memory=1 and trips the refcount assertion of debug PHP
builds. Z_TRY_ADDREF_P leaves non-refcounted strings alone.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EKX5yrrUnnbeRtc2oURC2u
…esolver

The offset-write branch wrapped a borrowed inner table with a bare
ZVAL_ARR and addref'ed it, the pattern that faults when the table is
immutable. The keysBySlot entries are always separated tables today, so
nothing crashes, but zv::Arr::copyOfTable() handles the immutable case
and says what the code means.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EKX5yrrUnnbeRtc2oURC2u
The snapshot was taken after separating, so it shared the table the loop
deletes from: the deletes went into a table of refcount 2 that was also
being iterated. Only the current entry is ever deleted, so the result
was right, but the write violated copy-on-write and trips HT_ASSERT_RC1
on debug PHP builds. Taking the snapshot first makes separate() give the
deletes their own table, at the same one duplication as before.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EKX5yrrUnnbeRtc2oURC2u
@ondrejmirtes
ondrejmirtes force-pushed the turbo-immutable-refcount-fixes branch from 05d811f to 01c272f Compare September 23, 2026 10:24
@ondrejmirtes
ondrejmirtes merged commit 01c272f into 2.3.x Sep 23, 2026
494 of 497 checks passed
@ondrejmirtes
ondrejmirtes deleted the turbo-immutable-refcount-fixes branch September 23, 2026 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants