pass ddtrace_globals explicitly through sidecar/telemetry GSHUTDOWN#3886
Merged
Conversation
In ZTS mode, ts_free_id iterates all per-thread TSRM storage slots from the main thread, calling zm_globals_dtor_ddtrace for each one. The dtor receives the correct zend_ddtrace_globals* for the dead worker thread, but ddtrace_sidecar_gshutdown() and ddtrace_telemetry_flush_bgs_metrics_if_due() were using DDTRACE_G() which reads the thread's TSRM slot. Once ts_free_id has processed the main thread's slot and nulled it, subsequent calls to DDTRACE_G(sidecar) dereference NULL → SIGSEGV. Fix: thread zend_ddtrace_globals* explicitly through both functions and replace all DDTRACE_G(field) with ddtrace_globals->field. Add DDTRACE_GLOBALS_PTR() macro for the non-GSHUTDOWN call site in ddtrace_telemetry_finalize(). Add ZtsGshutdownTests integration test (runs on ZTS + -PcheckCoreDumps) that reproduces the crash via MaxConnectionsPerChild 1 and verifies no core dump is produced. Enable -PcheckCoreDumps in the appsec integration CI jobs. Also wire up core-dump detection infrastructure in AppSecContainer (ulimit, core_pattern, copy cores to test-logs, detect on close). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
✨ Fix all issues with BitsAI or with Cursor
|
Benchmarks [ tracer ]Benchmark execution time: 2026-05-19 02:53:43 Comparing candidate commit 527a671 in PR branch Found 2 performance improvements and 1 performance regressions! Performance is the same for 191 metrics, 0 unstable metrics. scenario:ComposerTelemetryBench/benchTelemetryParsing
scenario:ContextPropagationBench/benchInject64Bit
scenario:PDOBench/benchPDOOverhead-opcache
|
Doesn't fix the fact this is called repeatedly in the main thread. When called by `ts_free_id` the threads whose data we want to free are gone. For this to work propoerly we would need to register an actual thread data destructor.
estringana
approved these changes
May 19, 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
2 participants
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.
In ZTS mode,
ts_free_iditerates all per-thread TSRM storage slots from the main thread, callingzm_globals_dtor_ddtracefor each one. The dtor receives the correctzend_ddtrace_globals*for the dead worker thread, butddtrace_sidecar_gshutdown()andddtrace_telemetry_flush_bgs_metrics_if_due()were usingDDTRACE_G()which reads the thread's TSRM slot. Oncets_free_idhas processed the main thread's slot and nulled it, subsequent calls toDDTRACE_G(sidecar)dereference NULL → SIGSEGV.Fix: thread
zend_ddtrace_globals*explicitly through both functions and replace allDDTRACE_G(field)withddtrace_globals->field. AddDDTRACE_GLOBALS_PTR()macro for the non-GSHUTDOWN call site inddtrace_telemetry_finalize().Add
ZtsGshutdownTestsintegration test (runs on ZTS + -PcheckCoreDumps) that reproduces the crash via MaxConnectionsPerChild 1 and verifies no core dump is produced. Enable -PcheckCoreDumps in the appsec integration CI jobs. Also wire up core-dump detection infrastructure in AppSecContainer (ulimit, core_pattern, copy cores to test-logs, detect on close).The test fails around 80% of the time on my machine.
Reviewer checklist