Commit 94686da
fix(context): scope OTEL ThreadContext storage to the carrier thread (PROF-15271) (#625)
* fix(context): scope OTEL ThreadContext storage to the carrier thread (PROF-15271)
The ThreadContext DirectByteBuffer is a window into the OTEP record embedded in
the *carrier's* native ProfiledThread — the record the (carrier-bound) sampler
reads. It was cached in a plain ThreadLocal, keying it by the *virtual* thread
and pinning it to whichever carrier was mounted at first use. That is wrong once
the vthread migrates (writes land on the old carrier, so a sampler on the new
carrier sees stale/empty context) and unsafe once the old carrier's OS thread
exits: the record is freed while the buffer keeps being written — a use-after-free
that can corrupt JVM-owned native memory (observed as a crash in
ThreadsSMRSupport::free_list).
Introduce a context-storage mode. In CARRIER mode, storage is backed by
jdk.internal.misc.CarrierThreadLocal (JDK 21+), whose get()/set()/remove()
operate on the current carrier's map even from a mounted virtual thread, so a
vthread always resolves to its current carrier's live record. Storage lifetime
then matches the native record's lifetime, eliminating the dangling-buffer window.
- OtelContextStorage: factory + Mode enum + kill-switch. The internal type is
built reflectively and held as its ThreadLocal supertype, so calls dispatch
virtually with no per-call reflection and no compile-time dependency (the lib
is a Java 8 baseline). Degrades to a plain ThreadLocal — today's behavior —
when the type is missing (older JDK) or inaccessible (export not granted),
never failing hard. Selected via -Dddprof.context.storage.mode=auto|carrier|thread.
- JavaProfiler: field uses the factory; a currentContext() get-or-init helper
replaces ThreadLocal.withInitial (a reflectively-built CarrierThreadLocal cannot
carry a supplier); the 8 context write sites route through it; contextStorageMode()
added for diagnostics/tests.
- Build: add --add-exports java.base/jdk.internal.misc=ALL-UNNAMED to the test JVM,
gated on testJvmMajorVersion() >= 21 (the flag aborts a Java 8 JVM). Production
grants the export from the agent (follow-up).
Tests:
- CarrierContextStorageTest: 2000 vthreads resolve to exactly one ThreadContext
per carrier (far fewer than the vthread count).
- OtelContextStorageTest: kill-switch forces plain ThreadLocal; auto selects
carrier when available; explicit carrier falls back without throwing.
- context_uaf_ut.cpp: ASan proof of the unguarded write-after-free primitive.
- Existing OtelContextStorageModeTest unregressed.
Follow-ups: export grant in dd-trace-java; reapply-on-mount / clear-on-unmount
coherence (PROF / #11646), which this change makes safe.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(context): fail hard on mode=carrier when CarrierThreadLocal is unavailable
Silently falling back to thread-scoped storage on a JDK 21+ JVM re-exposes the
exact virtual-thread context use-after-free this change removes. Make the policy
explicit at the mode level:
- carrier: carrier scoping is required — create() now throws if
jdk.internal.misc.CarrierThreadLocal is not accessible, with an actionable
message (add the --add-exports, or opt into mode=thread). This is the fail-fast
callers get once the runtime export is guaranteed.
- auto (default): still degrades so profiling loads even without the export
(the export may be granted by an agent that loads after this library, and the
fallback is safe for non-Loom apps), but now logs a loud WARN on JDK 21+ where
the fallback is unsafe under virtual threads. Silent on JDK < 21 (expected).
- thread: unchanged (legacy, silent).
Default stays graceful because at init we only know the JVM is Loom-capable, not
whether virtual threads route context here; failing hard by default would break
profiler startup for every JDK 21+ deployment until the agent-side export grant
lands. The tracer opts into mode=carrier once it guarantees the export.
Tests: carrierModeThrowsWhenUnavailable (fail-fast) and carrierModeUsesCarrierWhenAvailable
replace the previous graceful-fallback-on-carrier test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(context): address PR review — remove motivation-only cpp test, harden test & parsing
- Remove ddprof-lib/src/test/cpp/context_uaf_ut.cpp: it proved the native
write-after-free *primitive*, not this PR's Java carrier-scoping fix (its
red->green flip referenced a hypothetical native guard). The fix mechanism is
validated by CarrierContextStorageTest; a deterministic end-to-end UAF test
isn't feasible, so the danger stays documented in the PR/commit rationale.
- CarrierContextStorageTest: fix the stale "two touches with a park" comment to
match the single-touch reality, and relax the per-carrier assertion from
==1 to >=1. The map is keyed by carrier *name*, so a retired-and-replaced
ForkJoinPool worker reusing a slot name could legitimately expose a second
context; the did-NOT-key-by-vthread guarantee is now carried by the aggregate
bound (context count well below vthread count), which is name-stability-free.
- OtelContextStorage: parse the mode property with toLowerCase(Locale.ROOT) so
"CARRIER" is not mangled to "carrıer" under tr_TR (which would silently drop
the fail-fast semantics).
- JavaProfiler.getThreadContext(): document that the returned ThreadContext must
not be cached across a possible carrier migration (unmount/remount) — the
buffer targets the carrier mounted at call time; setContext* re-fetch per use.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(build): gate carrier --add-exports on the test JVM at execution time
The musl split-JDK CI (build JDK 21 via JAVA_HOME, test JDK 8 via JAVA_TEST_HOME)
failed with "Unrecognized option: --add-exports=java.base/jdk.internal.misc=ALL-UNNAMED"
on the JDK 8 test JVM.
Root cause: the flag was gated on PlatformUtils.testJvmMajorVersion() inside
ProfilerTestExtension.init{}, i.e. at Gradle *configuration* time. JAVA_TEST_HOME
is not resolvable then (that is precisely why the task executables are assigned in
doFirst, "so environment variables are read correctly"), so testJavaHome() fell back
to the build JDK (JAVA_HOME=21) and added a JDK-21-only flag that then aborted the
JDK-8 launcher.
Move the gate to task execution time: a carrierExportJvmArgs() helper evaluated in
both test doFirst blocks (glibc Test task and musl Exec task), where the real test
JVM (JAVA_TEST_HOME) is resolvable. When it is < 21 the flag is omitted and the
profiler degrades to thread-scoped storage (carrier tests skip) — safe.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(build): detect test-JVM version from the release file, not by exec
The previous execution-time gate still put --add-exports on the JDK 8 musl test
JVM: PlatformUtils.testJvmMajorVersion() launches `$JAVA_TEST_HOME/bin/java
-version`, and in the split-JDK musl matrix that probe reported >= 21 even though
the resolved executable (and the JVM that then rejected the flag) was JDK 8.
Read the major version from the test JDK's `release` file (JAVA_VERSION=...)
instead of executing the launcher: a pure file read of the same JAVA_TEST_HOME the
executable is resolved from — deterministic, no subprocess, no exec-format/PATH
hazards. Returns 0 when it can't be determined (missing/old `release`), so the flag
is omitted and carrier tests skip — fail-safe, never an abort. Adds a lifecycle log
of the resolved testJavaHome + detected major to make the gate decision visible.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore(build): lower carrier --add-exports gate log from lifecycle to info
The gate decision was logged at lifecycle level during stabilization to make the
musl/ibm CI behavior visible. The fix is proven (musl-8 green, ibm-8 flaky and
green on rerun), so drop it to info to avoid printing on every test invocation.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(context): address review — typed ContextStorageMode, ddprof.debug.* property, tidy create()
Addresses Jaroslav's review on PR #625:
- contextStorageMode() returns a typed ContextStorageMode (new public enum) instead
of a String; the OtelContextStorage factory stays package-private.
- Rename the selector to ddprof.debug.context.storage.mode (ddprof.debug.*, like
ddprof.debug.malloc_arena_max) to signal it is an internal knob, not supported config.
- create() parses the selector once into locals and documents that it runs once per
JavaProfiler instance (the tlsContextStorage field initializer), not per thread —
clarifying the review's per-thread concern.
Preserves the prior commits' work (Locale.ROOT parsing, getThreadContext caching note,
build-side --add-exports gating, removed motivation-only cpp test).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Jaroslav Bachorik <jaroslav.bachorik@datadoghq.com>1 parent 934f110 commit 94686da
6 files changed
Lines changed: 564 additions & 14 deletions
File tree
- build-logic/conventions/src/main/kotlin/com/datadoghq/profiler
- ddprof-lib/src/main/java/com/datadoghq/profiler
- ddprof-test/src/test/java/com/datadoghq/profiler
- context
Lines changed: 67 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
18 | 19 | | |
19 | 20 | | |
20 | 21 | | |
| |||
76 | 77 | | |
77 | 78 | | |
78 | 79 | | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
79 | 135 | | |
80 | 136 | | |
81 | 137 | | |
| |||
238 | 294 | | |
239 | 295 | | |
240 | 296 | | |
| 297 | + | |
| 298 | + | |
241 | 299 | | |
242 | 300 | | |
243 | 301 | | |
| |||
302 | 360 | | |
303 | 361 | | |
304 | 362 | | |
| 363 | + | |
| 364 | + | |
305 | 365 | | |
306 | 366 | | |
307 | 367 | | |
| |||
661 | 721 | | |
662 | 722 | | |
663 | 723 | | |
664 | | - | |
| 724 | + | |
| 725 | + | |
| 726 | + | |
| 727 | + | |
| 728 | + | |
| 729 | + | |
| 730 | + | |
665 | 731 | | |
666 | 732 | | |
667 | 733 | | |
| |||
Lines changed: 28 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
Lines changed: 58 additions & 13 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
39 | 39 | | |
40 | 40 | | |
41 | 41 | | |
42 | | - | |
43 | | - | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
44 | 67 | | |
45 | 68 | | |
46 | 69 | | |
| |||
191 | 214 | | |
192 | 215 | | |
193 | 216 | | |
194 | | - | |
| 217 | + | |
195 | 218 | | |
196 | 219 | | |
197 | 220 | | |
| |||
203 | 226 | | |
204 | 227 | | |
205 | 228 | | |
206 | | - | |
| 229 | + | |
207 | 230 | | |
208 | 231 | | |
209 | 232 | | |
210 | 233 | | |
211 | 234 | | |
212 | 235 | | |
213 | 236 | | |
214 | | - | |
| 237 | + | |
215 | 238 | | |
216 | 239 | | |
217 | 240 | | |
| |||
226 | 249 | | |
227 | 250 | | |
228 | 251 | | |
229 | | - | |
| 252 | + | |
230 | 253 | | |
231 | 254 | | |
232 | 255 | | |
| |||
236 | 259 | | |
237 | 260 | | |
238 | 261 | | |
239 | | - | |
| 262 | + | |
240 | 263 | | |
241 | 264 | | |
242 | 265 | | |
| |||
263 | 286 | | |
264 | 287 | | |
265 | 288 | | |
266 | | - | |
| 289 | + | |
267 | 290 | | |
268 | 291 | | |
269 | 292 | | |
270 | | - | |
| 293 | + | |
271 | 294 | | |
272 | 295 | | |
273 | 296 | | |
| |||
444 | 467 | | |
445 | 468 | | |
446 | 469 | | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
| 476 | + | |
| 477 | + | |
| 478 | + | |
| 479 | + | |
| 480 | + | |
| 481 | + | |
447 | 482 | | |
448 | | - | |
| 483 | + | |
| 484 | + | |
| 485 | + | |
| 486 | + | |
| 487 | + | |
| 488 | + | |
| 489 | + | |
| 490 | + | |
| 491 | + | |
| 492 | + | |
449 | 493 | | |
450 | 494 | | |
451 | 495 | | |
| |||
459 | 503 | | |
460 | 504 | | |
461 | 505 | | |
462 | | - | |
463 | | - | |
464 | | - | |
| 506 | + | |
| 507 | + | |
| 508 | + | |
| 509 | + | |
465 | 510 | | |
466 | 511 | | |
467 | 512 | | |
| |||
0 commit comments