[PROF-15201] fix(profiler): close two TOCTOU races between SIGPROF handler and JFR lifecycle#614
[PROF-15201] fix(profiler): close two TOCTOU races between SIGPROF handler and JFR lifecycle#614r1viollet wants to merge 5 commits into
Conversation
… lifecycle The CPU profiler sends SIGPROF to all threads via per-thread kernel timers. The signal handler checks _enabled and, if true, calls recordSample() which accesses JFR buffers. Two races existed around the recording cycle transition (default every 60 s) where JFR structures could be in mid-init or mid-teardown while the handler was active: Race 1 — stop() side (TOCTOU on _enabled vs _jfr.stop()): A handler that passed the _enabled=true check could still be executing inside recordSample() when disableEngines() set _enabled=false and _jfr.stop() freed JFR buffers — use-after-free → SIGSEGV. Fix: add an _inflight counter (incremented on handler entry, decremented on all exits). CTimer::stop() calls drainInflight() after deleting per- thread timers, spinning until _inflight==0 before returning to the caller that proceeds to _jfr.stop(). Any handler that fires after disableEngines() sees _enabled=false and returns early without touching JFR. Race 2 — start() side (enableEngines() before _jfr.start()): enableEngines() set _enabled=true before _jfr.start() had completed. A SIGPROF in that window would see _enabled=true and call recordSample() on partially-initialized JFR structures. Fix: move enableEngines() to after _jfr.start() and _cpu_engine->start() have both returned successfully (just before _state.store(RUNNING)). Validated empirically: a controlled reproducer in DataDog/profiling-backend (AnalysisEndpointTest.testResourceExhausted with a 60 s recording period) showed ~60% failure rate without the fix (SIGSEGV / hang), 0% with both fixes applied (12/12 iterations clean). Each fix alone only partially addressed the failures, confirming both races were independently active. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
CI Test ResultsRun: #28235552371 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Failed Testsmusl-aarch64/debug / 11-librcaJob: View logs No detailed failure information available. Check the job logs. musl-aarch64/debug / 8-librcaJob: View logs No detailed failure information available. Check the job logs. musl-aarch64/debug / 17-librcaJob: View logs No detailed failure information available. Check the job logs. musl-aarch64/debug / 21-librcaJob: View logs No detailed failure information available. Check the job logs. musl-aarch64/debug / 25-librcaJob: View logs No detailed failure information available. Check the job logs. Summary: Total: 32 | Passed: 27 | Failed: 5 Updated: 2026-06-26 11:53:37 UTC |
Reliability & Chaos Results❌ 2 failure(s) detected Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/120504164 ❌ chaos: profiler gmalloc amd64 21 0 3 temXchaos❌ chaos: profiler jemalloc aarch64 25 0 3 temXchaos |
…iveness Without a timeout, drainInflight() spins indefinitely if a SIGPROF handler is stuck (e.g. deadlock inside recordSample()). This would prevent _jfr.stop() from running and hang JVM shutdown. Use clock_gettime(CLOCK_MONOTONIC) for a real wall-clock bound of 200ms. If the deadline fires, log a warning and proceed; in that pathological case the caller's JFR teardown may race with the stuck handler, but the JVM is not permanently deadlocked. In normal operation (handlers complete in microseconds) the timeout is never reached. Refs: PROF-15201 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81184abc5c
ℹ️ 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".
…Guard Manual increment/decrement of _inflight in signal handlers was error-prone and led to a counter leak in CTimerJvmti::signalHandler's inInitWindow() early return path. This would cause drainInflight() to spin for 200ms on every profiler stop and emit spurious warnings about stuck handlers. Solution: Add InflightGuard RAII class following the existing pattern in guards.h (SignalHandlerScope, CriticalSection). The guard increments on construction and decrements on destruction, making it impossible to forget the decrement on any exit path. Benefits: - Eliminates the entire class of counter-leak bugs - Makes all exit paths safe by construction - Follows established patterns in the codebase (SignalHandlerScope) - Self-documenting: InflightGuard at the start of a handler clearly signals its purpose Changes: - guards.h: Add InflightGuard class declaration - guards.cpp: Implement InflightGuard with #ifdef __linux__ (no-op on non-Linux where CTimer doesn't exist) - ctimer_linux.cpp: Replace all manual __atomic_fetch_add/_sub with a single InflightGuard declaration at the start of both signal handlers
|
I'm worried about perf of such an approach. |
Scan-Build Report
Analyzer FailuresThe analyzer had problems processing the following files:
Please consider submitting preprocessed files as . |
CI failure revealed _inflight was protected, preventing InflightGuard access. Also, having _inflight adjacent to _enabled creates false sharing: - _enabled is read (ACQUIRE) by every signal handler on every thread - _inflight is written (modify-release) by every signal handler Sharing a cache line causes unnecessary cache line bouncing. Solution: Move _inflight to public and align to 64-byte cache line boundary (alignas(64)). This separates the read-only _enabled hot path from the read-write _inflight counter, reducing cross-thread traffic. Note: The counter is still globally updated, but the separate cache line means _enabled reads no longer compete with _inflight writes.
d28b55d to
938e91d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 938e91da7d
ℹ️ 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".
| "This may indicate a stuck signal handler.", | ||
| DRAIN_TIMEOUT_NS / 1000000L, | ||
| __atomic_load_n(&_inflight, __ATOMIC_ACQUIRE)); | ||
| break; |
There was a problem hiding this comment.
Keep JFR alive while SIGPROF handlers are in flight
When a SIGPROF handler is still inside recordSample()/recordSampleDelegated() for more than 200 ms, this break lets CTimer::stop() return with _inflight > 0; Profiler::stop() can then call _jfr.stop() and free the buffers that the handler is still using, recreating the use-after-free this guard is meant to prevent. If a liveness bound is required, the stop path should avoid tearing down JFR while the counter is non-zero rather than proceeding after the warning.
Useful? React with 👍 / 👎.
…use-after-free Codex P2: The 200ms timeout in drainInflight() previously broke with a warning but still allowed Profiler::stop() to call _jfr.stop(), recreating the use-after-free this guard is meant to prevent. Solution: drainInflight() returns bool; Profiler::stop() skips _jfr.stop() if timeout fires. This leaks ~few MB but prevents SIGSEGV. Timeout is pathological (stuck handler), so leak is acceptable. Tested: forcing skip-stop works cleanly. Normal operation unchanged: _jfr.stop() is called when drain succeeds (<1ms). Only skips on timeout (200ms expired, handler still in-flight).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36ce1bda9a
ℹ️ 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".
There was a problem hiding this comment.
Keep stop non-restartable when drain fails
When drainInflight() times out, this branch leaves the existing FlightRecorder recording alive but Profiler::stop() still falls through to _state.store(IDLE) and returns OK, so the rotation path or a user can immediately call start() while an old SIGPROF handler is still in flight. FlightRecorder::start()/newRecording() then installs a new _rec without having stopped the old one, and the old handler can later record into the new recording (or leak/overwrite the old one). Fresh evidence in this revision is that the false branch now skips _jfr.stop() but still treats the profiler as fully stopped; in the timeout case it should return an error or otherwise keep the profiler non-restartable until the in-flight handler is gone.
Useful? React with 👍 / 👎.

What does this PR do?:
Closes two TOCTOU races between the SIGPROF signal handler and JFR lifecycle transitions that could cause SIGSEGV or hangs in the test JVM during the 60-second recording cycle rotation.
Race 1 — stop() side (
ctimer_linux.cpp):disableEngines()sets_enabled=false, but a handler that already passed the_enabled=truecheck could still be executing insiderecordSample()when_jfr.stop()freed JFR buffers → use-after-free → SIGSEGV (or hang if the crash is caught by crashtracking).Fix: add an
_inflightcounter, incremented on every handler entry before the_enabledcheck, decremented on every exit path.CTimer::stop()callsdrainInflight()after deleting per-thread timers, spinning until_inflight==0before returning. The caller (Profiler::stop) then proceeds to_jfr.stop()only once all handlers have fully exited.Race 2 — start() side (
profiler.cpp):enableEngines()set_enabled=truebefore_jfr.start()had completed. A SIGPROF delivered in that window would see_enabled=trueand callrecordSample()on partially-initialized JFR structures.Fix: move
enableEngines()to after both_jfr.start()and_cpu_engine->start()have returned successfully (immediately before_state.store(RUNNING)).Motivation:
Discovered while investigating intermittent SIGSEGV (exit 139) and hang failures in DataDog/profiling-backend CI. Bisected to a dd-trace-java commit that changed instrumentation initialization timing, shifting when the 60-second recording cycle boundary fell relative to test thread activity — exposing both races reliably enough to isolate.
How to test the change?:
Controlled reproducer in DataDog/profiling-backend using
AnalysisEndpointTest.testResourceExhaustedwith the bad dd-trace-java agent (0e13e90dac) and a patchedlibjavaProfiler.so:drainInflight): ~20% failure rate — Race 2 still activeenableEngines): ~40% failure rate — Race 1 still activev_1.44.0baselineAdditional Notes:
drainInflight()is an unbounded spin. In practicerecordSample()completes in microseconds so this is safe, but a bounded spin with a log warning could be added as a follow-up._inflightcounter is incremented even whenCriticalSectionfails (handler returns early without touching JFR). This is intentional: it makes the drain conservative and guarantees the counter reaches zero only after all code paths between the counter increment and any potential JFR access have completed.For Datadog employees: