fix(appsec): musl ZTS load + ZTS thread-exit teardown; ci: coverage disk envelope - #4180
Conversation
…n load
The TLS destructor registration in PHP_GINIT_FUNCTION(ddappsec) declared
__cxa_thread_atexit_impl as a strong symbol and called it unconditionally on
Linux. The symbol is glibc-private and musl exports it nowhere, and musl
resolves relocations eagerly, so dlopen("ddappsec.so") fails outright with
"Error relocating ...: __cxa_thread_atexit_impl: symbol not found". AppSec is
therefore dead on musl + ZTS (FrankenPHP-Alpine, Swoole/pthreads on Alpine) in
1.25.0 and 1.25.1. ddtrace-zts.so references the same symbol weakly, which is
why the tracer is unaffected.
Declare it weak and register the destructor only when it resolves.
registered_thread_local_dtor then stays false on musl, which is the case
PHP_GSHUTDOWN_FUNCTION(ddappsec) already handles ("a platform without a
thread-exit destructor mechanism"), so tshutdown still runs on the owning
thread. glibc is unchanged: the weak reference resolves there and the
destructor is registered as before.
Verified on dunglas/frankenphp:php8.3.12-alpine (PHP 8.3.12 ZTS, musl 1.2.5),
same tree, incremental rebuild of only this file:
before: NOTYPE GLOBAL DEFAULT UND __cxa_thread_atexit_impl
extension_loaded("ddappsec") => false, "Error relocating" warning
after: NOTYPE WEAK DEFAULT UND __cxa_thread_atexit_impl
extension_loaded("ddappsec") => true, no relocation error
…overage "helper-rust integration coverage" never set DOCKER_LOOPBACK_SIZE, so it ran on the DinD default of ~20G while its sibling .appsec_integration_tests sets 30G and "push appsec images" sets 100G. 17 consecutive master runs died in :buildPortableLibdatadogPhp with "failed to build archive at .../libdatadog_php.a: No space left on device (os error 28)", and the 10 runs after that with "Bus error" from musl-clang at the final cdylib link. A same-pipeline A/B against the sibling holds everything else equal - same image, runner tag, CPU and memory - and differs only by -PuseHelperRustCoverage and the loopback size. Raise it to 50G: the coverage build is strictly larger than the sibling's 30G. The existing rm -rf /vol/cargo-target mitigation sits after the build, so it never runs when the build itself is what exhausts the volume. Also report df -h /, docker system df and the php-portable-libdatadog-php volume before and after the build. The ENOSPC is measured; the "Bus error" being the same exhaustion one step later is still an inference, and these numbers settle it in one cycle. The gradle exit status is preserved so a failing build still fails the job, and the report is emitted on that path too, which is the case the numbers are for.
|
Benchmarks [ appsec ]Benchmark execution time: 2026-09-15 10:05:04 Comparing candidate commit 83a2260 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics.
|
__cxa_thread_atexit_impl is a glibc internal, so the weak reference from 8b91eda resolves to NULL on musl and no destructor was registered at all, leaking each exited thread's entity-body buffer and cached telemetry strings. Fall back to a pthread key, as libc++ and libstdc++ do for __cxa_thread_atexit; unlike __cxa_thread_atexit_impl it takes no reference on the DSO, which costs nothing since musl never unmaps one. On its own that would trade the leak for a crash: _tshutdown_handler() reads this thread's config (mlog_g -> dd_log_level -> zai_config_get_value), which PHP has already freed by the time a thread-exit destructor runs, so having GSHUTDOWN stand down whenever a destructor was registered faults as soon as GSHUTDOWN does run on the owning thread -- as it already does on glibc at 8b91eda. Run the handler at whichever of the two fires first, guarded by a native thread-local, so it runs exactly once per thread and always while that thread's config is still mapped.
|
I like it now. Next week I'll also add an appsec integration tsts against musl zts so this doesn't recur. |
Benchmarks [ tracer ]Benchmark execution time: 2026-09-14 17:45:20 Comparing candidate commit f278620 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 193 metrics, 1 unstable metrics.
|
5f58771 replaced the compile-time libc check with a weak gnu_get_libc_version probe, leaving no reader of the define.
Leiyks
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dda85f021b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
3 participants
Description
One commit per change; they share no code and can be reviewed separately.
fix(appsec): weakly reference__cxa_thread_atexit_impl. musl exports no such symbol (it is a glibc 2.18 internal) and resolves relocations eagerly, so the strong reference madedlopen("ddappsec.so")fail outright. Every ZTS build emits it, so the failure set ismusl ∩ ZTS— already shipped in 1.25.0 and 1.25.1, where AppSec is therefore fully non-functional on Alpine/FrankenPHP ZTS, so this needs to be in the next release.fix(appsec): pthread fallback for the thread-exit destructor. Without that symbol nothing tears non-main threads down, so GINIT falls back to apthread_key_createdestructor and aZEND_TLSflag lets whichever of GSHUTDOWN-on-the-owning-thread or thread-exit comes first do the work. That ordering also fixes a segfault reproducible on unmodified master (_tshutdown_handler→dd_log_level→zai_config_get_valueafter PHP freed the thread's config;pecl parallel, 3Runtimes thenclose()), which the oldregistered_thread_local_dtorhandshake caused by leaving thread-exit as the only teardown path on glibc ZTS.ci: disk envelope forhelper-rust integration coverage. The job never setDOCKER_LOOPBACK_SIZE, so it ran on the DinD default of ~20G; the addeddfprobes measure the volume going 3.2G → 19.9G across the build, leaving ~0.1G of headroom there, hence the alternation betweenNo space left on deviceandBus error. Raised to 50G, and the job now passes.Test infrastructure (separate author). Portable
dd_library_loaderin the appsec integration tests, a runtimegnu_get_libc_versionprobe replacing the compile-time__MUSL__so one loader build serves both libcs, and an Apache ZTS musl AppSec test image so this combination is covered at all; a final commit drops the now-dead__MUSL__detection.installer testsstays red. It installs the published tarball for$(cat VERSION)and itsneedssupply only*-unknown-linux-gnubundles, so the musl-ZTS sub-test keeps pulling 1.25.1 whatever master contains; only a release carrying the first fix clears it.