Never patch the profiler's own import table

[hisener]

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

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.

patch_socket_functions() had the same hazard by another route. It computed its is-self flags in a pre-pass keyed by library index, then applied them in a second, locked pass that re-read native_libs.at(index). The array can grow between the two passes, so a flag could be applied to a different CodeCache than the one it was computed for — and a stale false on our own library would let us patch ourselves. That pre-pass only existed because the old check called realpath(), which must not run while holding _lock; the range check has no such constraint, so it now runs on the library actually being patched and the bool is_self[MAX_NATIVE_LIBS] array is gone.

_profiler_name and initialize()

_profiler_name is no longer used for identification and is removed.

initialize() stays, now setting an explicit flag rather than that string. Those _profiler_name == nullptr checks were doing double duty: besides the path comparison they gated patching on the profiler being up. Profiler::start() calls initialize() immediately before the first updateSymbols(), so in production the guard is always satisfied by then — but remove it and any earlier Libraries refresh installs pthread_create_hook before there is a profiler to register threads with. The reproducer then crashes deterministically:

SIGSEGV  libjavaProfiler.so+0x3a300  Profiler::registerThread(int)+0x20

(The full gtest suite passes with the guard removed, so tests alone do not catch this. The // only happens in Gtest comment on that guard is misleading.)

The flag is std::atomic<bool> with release/acquire, matching _socket_active: it is written from Profiler::start() and read from the Libraries refresher thread via patch_libraries(). initialize() resets _size before the release store, so a thread that observes the flag also observes the reset.

Note the gate is deliberately not added to patch_socket_functions(): that is only reachable from NativeSocketSampler::start() and from install_socket_hooks(), which early-returns unless _socket_active (false until the first batch), so it cannot run before start — and gating it would risk silently disabling socket patching if NativeSocketSampler::start() were ever ordered before LibraryPatcher::initialize().

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).

Possible 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

[zhengyu123]

_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.

[zhengyu123]

LGTM

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[zhengyu123]

Actually, there is a similar problem in LibraryPatcher::patch_socket_functions()

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

ddprof-lib/src/main/cpp/libraryPatcher_linux.cpp:433

• This _initialized check needs an atomic load once _initialized is changed to std::atomic<bool>; otherwise it won’t compile and the intended cross-thread gate won’t be correctly synchronized.

void LibraryPatcher::patch_libraries() {
   // Profiler::start() has not run yet, so the hook would have nowhere to
   // register new threads. Also the case in Gtest, which never initializes.
   if (!_initialized) {
     return;
   }

ddprof-lib/src/main/cpp/libraryPatcher_linux.cpp:511

• This _initialized check needs an atomic load once _initialized is changed to std::atomic<bool>; otherwise it won’t compile and the intended cross-thread gate won’t be correctly synchronized.

void LibraryPatcher::patch_sigaction_in_library(CodeCache* lib) {
  if (lib->name() == nullptr) return;
  if (!_initialized) return;  // Not initialized yet

[hisener]

Actually, there is a similar problem in LibraryPatcher::patch_socket_functions()

@zhengyu123 Claude made more changes to apply your comment and Codex comments: https://github.com/DataDog/java-profiler/compare/19572a3ef622ab14cd6715f616a91355ba2bc791..e4225cf51eb5f61ec3911f5cd6c5e0d544950ff7

[zhengyu123]

LGTM

[rkennke]

Looks good to me, thank you!

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

ddprof-lib/src/test/cpp/libraryPatcher_ut.cpp:60

• anchorMin()/anchorMax() do pointer arithmetic on a const char* derived from a function address. In C++ this is undefined behavior (pointer arithmetic is only defined within the same object/array), and can trip UBSan or stricter toolchains. Prefer doing the offset math via uintptr_t and cast back to const void*.

const void* anchorMin() {
  return (const char*)LibraryPatcherTestAccessor::selfAnchor() - 0x1000;
}