Skip to content

Never patch the profiler's own import table - #721

Draft
hisener wants to merge 1 commit into
DataDog:mainfrom
hisener:halil.sener/fix-library-patcher-self-patch
Draft

Never patch the profiler's own import table#721
hisener wants to merge 1 commit into
DataDog:mainfrom
hisener:halil.sener/fix-library-patcher-self-patch

Conversation

@hisener

@hisener hisener commented Aug 6, 2026

Copy link
Copy Markdown
Member

This PR stops LibraryPatcher from patching the profiler's own import table, which could turn pthread_create_hook() into unbounded recursion and kill the JVM.

patch_library_unlocked() recognised its own library by comparing realpath(lib) with the profiler's path, and skipped that comparison entirely when realpath() returned nullptr (libraryPatcher_linux.cpp#L419-L426). dd-trace-java extracts libjavaProfiler.so to a temporary file and unlinks it once loaded, so realpath() on the still-mapped path fails, the self-check reports "not self", and a library re-scan patches our own GOT entry for pthread_create. pthread_create_hook() reaches the real pthread_create() through that same entry, so the hook then calls itself until the thread stack is exhausted: SIGSEGV, and no hs_err file, because crash reporting needs stack of its own. Whether it happens depends on a re-scan landing after the unlink, which is what made these crashes look random.

Diagnosed from core dumps of JVM test crashes:

  • the crashing thread's stack holds 3880-6136 identical pthread_create_hook+0xa8 frames (the return address of its own bl pthread_create@plt)
  • si_signo=11 si_code=128 (SI_KERNEL) si_addr=0x0, pc in malloc, and sp ~2MB below the thread's stack base
  • the profiler's own pthread_create JUMP_SLOT holds base+<pthread_create_hook> in every core inspected
  • crashing threads are always ones that create threads, during teardown, after every test passed

Reproduced at ~8-10% per run on a JVM test target with the profiler active. A standalone reproducer (unlink the extracted .so, dlopen to force a re-scan, then create threads) crashes dd-java-agent 1.65.0 downloaded straight from Maven Central, so this is not fixed in the latest release.

The fix recognises our own library by mapped address range, which cannot fail:

bool LibraryPatcher::is_profiler_library(CodeCache* lib) {
  return lib != nullptr && lib->contains(self_anchor());
}

self_anchor() returns the address of a function in this translation unit — a function rather than a static variable, because a CodeCache spans a library's executable segments (Symbols::parseLibraries builds the bounds from /proc/self/maps), which do not cover .data/.bss. Since every native library cache carries those bounds, there is no name comparison and no fallback: per review, falling back to something known to be faulty is not worth keeping, and the library deletion that triggers it is going away regardless.

Applied at all three patch sites — pthread_create, sigaction, and the socket functions — the last of which also passed a possibly-null _profiler_name to strcmp().

Tests — new libraryPatcher_ut.cpp (7 tests). They are verified to fail without the fix: swapping the original realpath/strcmp logic back in makes exactly RecognisesSelfWhenItsLibraryFileWasUnlinked and LeavesItsOwnPthreadCreateSlotUntouched fail. StillPatchesForeignPthreadCreateSlot passes in both states as a control, so the guard is shown to discriminate rather than to have quietly disabled patching. The full :ddprof-lib:gtestDebug suite passes (54 test binaries, no failures).

Also validated end to end against the reproducer, building the .so twice from this tree and swapping each into the same 1.65.0 agent jar:

library outcome
control (fix reverted, same tree) crashed on round 6 — SIGSEGV, self-patched GOT, 5863 recursive frames
fixed survived 200 rounds, no crash, no cores

The control matters as much as the fixed run: it shows a main-built library in a repackaged jar still crashes, so the repackaging is not what made the crash go away.

Worth considering as a follow-up: having pthread_create_hook() call the real function through a cached dlsym(RTLD_NEXT, ...) pointer instead of its own PLT — as patch_socket_functions() already does for send/recv/write/read — would make a self-patch harmless rather than merely prevented. Not included here to keep this change off the thread-creation hot path.


🤖 Generated with Claude Code

@hisener
hisener force-pushed the halil.sener/fix-library-patcher-self-patch branch from e29143e to 5c7c2d5 Compare August 6, 2026 15:34
@datadog-prod-us1-5

This comment has been minimized.

LibraryPatcher recognised its own library by comparing realpath(lib) with
the profiler's path, and skipped that comparison entirely when realpath()
returned nullptr. dd-trace-java extracts libjavaProfiler.so to a temporary
file and unlinks it once loaded, so realpath() on the still-mapped path
fails and the self-check reported "not self" - letting a library re-scan
patch our own GOT entry for pthread_create. pthread_create_hook() reaches
the real pthread_create() through that same entry, so the hook then called
itself until the thread stack was exhausted: SIGSEGV with no hs_err file,
since crash reporting needs stack of its own. Whether it happened depended
on a re-scan landing after the unlink, which made it look random.

Recognise our own library by mapped address range instead, which cannot
fail. Every native library cache carries its mapping bounds, so no name
comparison is kept as a fallback. Apply it at all three patch sites -
pthread_create, sigaction and the socket functions - the last of which
also passed a possibly-null _profiler_name to strcmp().

Environment: Datadog workspace

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hisener
hisener force-pushed the halil.sener/fix-library-patcher-self-patch branch from 5c7c2d5 to 64845c6 Compare August 6, 2026 17:24

@zhengyu123 zhengyu123 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

_profiler_name is no longer used, so please remove it. You can also remove LibraryPatcher::initialize(), as its sole purpose was to initialize _profiler_name.

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