Skip to content

fix(zai-config): dup INI values per ZTS thread instead of refcounting#3832

Merged
morrisonlevi merged 3 commits intomasterfrom
levi/fix-zai-ini-zts-refcount-race
Apr 28, 2026
Merged

fix(zai-config): dup INI values per ZTS thread instead of refcounting#3832
morrisonlevi merged 3 commits intomasterfrom
levi/fix-zai-ini-zts-refcount-race

Conversation

@morrisonlevi
Copy link
Copy Markdown
Collaborator

@morrisonlevi morrisonlevi commented Apr 26, 2026

SCP-1166

Description

Relates to #3729.

In ZTS mode, zai_config_ini_rinit was assigning zend_string_copy(source->value) to each thread's per-thread INI entry. This was a refcount-bump of the process-wide master zend_string stored in zai_config_memoized_entries. PHP's refcount operations are not atomic. With many ZTS workers each running rinit concurrently, the competing ++/-- on the shared refcount produced torn values, premature frees, and use-after-free reads, observed as zend_mm_heap corrupted or crashes under heavy FrankenPHP load, particularly on aarch64.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

…aring

zai_config_ini_rinit was replacing each ZTS thread's per-thread zend_string
(originally created by PHP's zend_copy_ini_directives via zend_string_dup)
with zend_string_copy(source->value) — a refcount-shared pointer to the
process-wide master in zai_config_memoized_entries[i].ini_entries[n]->value.

PHP's refcount ops are not atomic. With many ZTS workers each running this
rinit on every request, concurrent ++/-- on the shared refcount produced
torn values, premature frees, and use-after-free reads / Zend MM heap
corruption (observed as "zend_mm_heap corrupted" inside zend_string_release
during this very function on FrankenPHP under heavy load).

Match PHP's own thread-setup pattern: zend_string_dup gives each thread an
independent persistent allocation with refcount 1 that is never raced. In
the orig_value == ini->value aliased case, repoint ini->value to its own
fresh dup before releasing orig_value to avoid a dangling pointer. Memory
cost is ~tens of bytes per ZAI entry per thread; rinit churn is balanced
release+dup, no growth.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@datadog-datadog-prod-us1
Copy link
Copy Markdown

datadog-datadog-prod-us1 Bot commented Apr 26, 2026

Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 60.64% (+0.00%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: e60de64 | Docs | Datadog PR Page | Give us feedback!

@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented Apr 26, 2026

Benchmarks [ tracer ]

Benchmark execution time: 2026-04-27 15:42:52

Comparing candidate commit e60de64 in PR branch levi/fix-zai-ini-zts-refcount-race with baseline commit 7a66ef9 in branch master.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 193 metrics, 0 unstable metrics.

scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache

  • 🟩 execution_time [-9.335µs; -7.685µs] or [-8.317%; -6.847%]

Comment thread zend_abstract_interface/config/config_ini.c Outdated
@bwoebi
Copy link
Copy Markdown
Collaborator

bwoebi commented Apr 26, 2026

Alternatively, we might just intern the persistent zai config ini values (source->value) - they're never going to change after first_rinit here? Would that work?

@bwoebi bwoebi force-pushed the levi/fix-zai-ini-zts-refcount-race branch 4 times, most recently from 9047aa2 to 9d3d627 Compare April 27, 2026 13:36
@bwoebi bwoebi force-pushed the levi/fix-zai-ini-zts-refcount-race branch from 9d3d627 to 940690f Compare April 27, 2026 14:22
Copy link
Copy Markdown
Collaborator

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems stable now.

@bwoebi bwoebi marked this pull request as ready for review April 27, 2026 15:17
@bwoebi bwoebi requested a review from a team as a code owner April 27, 2026 15:17
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e60de64d6c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +412 to +416
if (identical_orig) {
ini->value = ini->orig_value;

if (!applied_update) {
if (ZaiConfigOnUpdateIni(ini, ini->value, NULL, NULL, NULL, PHP_INI_STAGE_RUNTIME) == SUCCESS) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Replay modified INI values when orig and value differ

On ZTS, this change now only calls ZaiConfigOnUpdateIni(...) inside if (identical_orig), so entries with ini->modified == true and ini->orig_value != ini->value no longer reapply their runtime INI override during zai_config_ini_rinit. That is a regression because zai_config_runtime_config_ctor() repopulates request config from globals, and earlier INI on_modify callbacks are ignored when runtime config is not initialized yet (ZaiConfigOnUpdateIni returns early). In requests that rely on user.ini/htaccess/FPM runtime INI overrides, config values can silently fall back to global defaults.

Useful? React with 👍 / 👎.

@morrisonlevi morrisonlevi merged commit e31741f into master Apr 28, 2026
1927 of 1933 checks passed
@morrisonlevi morrisonlevi deleted the levi/fix-zai-ini-zts-refcount-race branch April 28, 2026 14:54
@github-actions github-actions Bot added this to the 1.19.0 milestone Apr 28, 2026
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