From e4225cf51eb5f61ec3911f5cd6c5e0d544950ff7 Mon Sep 17 00:00:00 2001 From: "halil.sener" Date: Thu, 6 Aug 2026 14:34:39 +0000 Subject: [PATCH] Never patch the profiler's own import table 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. patch_socket_functions() had the same hazard by another route. It computed its is-self flags in a pre-pass keyed by library index and applied them in a second, locked pass; the array can grow in between, so a flag could be applied to the wrong entry and let us patch ourselves. The pre-pass only existed because the old check called realpath() and could not run under the lock, which no longer applies - the check now runs on the library actually being patched. _profiler_name is no longer used for identification, so drop it. The "initialized yet?" guards it doubled as are still needed and now read an explicit flag: patching must not start before Profiler::start(), because pthread_create_hook() routes new threads through Profiler::registerThread(), which crashes on a profiler that is not running. The flag is atomic with release/acquire, being written from Profiler::start() and read from the Libraries refresher thread. Environment: Datadog workspace Co-Authored-By: Claude Opus 5 (1M context) --- ddprof-lib/src/main/cpp/libraryPatcher.h | 14 +- .../src/main/cpp/libraryPatcher_linux.cpp | 75 ++++---- ddprof-lib/src/main/cpp/profiler.cpp | 3 +- ddprof-lib/src/test/cpp/libraryPatcher_ut.cpp | 170 ++++++++++++++++++ 4 files changed, 226 insertions(+), 36 deletions(-) create mode 100644 ddprof-lib/src/test/cpp/libraryPatcher_ut.cpp diff --git a/ddprof-lib/src/main/cpp/libraryPatcher.h b/ddprof-lib/src/main/cpp/libraryPatcher.h index 70be3659b7..97a3f26e3c 100644 --- a/ddprof-lib/src/main/cpp/libraryPatcher.h +++ b/ddprof-lib/src/main/cpp/libraryPatcher.h @@ -18,9 +18,16 @@ typedef struct _patchEntry { class LibraryPatcher { + friend class LibraryPatcherTestAccessor; + private: static SpinLock _lock; - static const char* _profiler_name; + // Set by initialize(), which Profiler::start() calls just before the first + // library scan. Patching must not begin any earlier: pthread_create_hook() + // routes newly created threads through Profiler::registerThread(), which + // dereferences state that only exists once the profiler is running. Read from + // the Libraries refresher thread, hence atomic with release/acquire. + static std::atomic _initialized; static PatchEntry _patched_entries[MAX_NATIVE_LIBS]; static int _size; static bool _patch_pthread_create; @@ -38,6 +45,11 @@ class LibraryPatcher { static void patch_pthread_create(); static void patch_pthread_setspecific(); static void patch_sigaction_in_library(CodeCache* lib); + // An address known to lie inside this library; how is_profiler_library() + // recognises the profiler's own mapping. + static const void* self_anchor(); + // True when `lib` is the profiler's own library, which must never be patched. + static bool is_profiler_library(CodeCache* lib); public: // True while socket hooks are installed; read by Profiler::dlopen_hook // to decide whether to re-patch after a new library is loaded. diff --git a/ddprof-lib/src/main/cpp/libraryPatcher_linux.cpp b/ddprof-lib/src/main/cpp/libraryPatcher_linux.cpp index e458fe6a1c..97e75822e7 100644 --- a/ddprof-lib/src/main/cpp/libraryPatcher_linux.cpp +++ b/ddprof-lib/src/main/cpp/libraryPatcher_linux.cpp @@ -12,18 +12,15 @@ #include "nativeSocketSampler.h" #include "profiler.h" -#include #include #include -#include #include #include -#include typedef void* (*func_start_routine)(void*); SpinLock LibraryPatcher::_lock; -const char* LibraryPatcher::_profiler_name = nullptr; +std::atomic LibraryPatcher::_initialized{false}; PatchEntry LibraryPatcher::_patched_entries[MAX_NATIVE_LIBS]; int LibraryPatcher::_size = 0; PatchEntry LibraryPatcher::_sigaction_entries[MAX_NATIVE_LIBS]; @@ -33,16 +30,40 @@ int LibraryPatcher::_socket_size = 0; std::atomic LibraryPatcher::_socket_active{false}; void LibraryPatcher::initialize() { - if (_profiler_name == nullptr) { - Dl_info info; - void* caller_address = __builtin_return_address(0); // Get return address of caller - bool ret = dladdr(caller_address, &info); - assert(ret); - _profiler_name = realpath(info.dli_fname, nullptr); + if (!_initialized.load(std::memory_order_acquire)) { _size = 0; + // Release so the reset above is visible to any thread that sees the flag. + _initialized.store(true, std::memory_order_release); } } +const void* LibraryPatcher::self_anchor() { + // Any code address in this translation unit lies inside the profiler's own + // mapping, so its address identifies us. A function is used rather than a + // static variable because a CodeCache spans a library's executable segments + // (see Symbols::parseLibraries), which do not cover .data/.bss. + return (const void*)&self_anchor; +} + +// The profiler must never patch its own import table. pthread_create_hook() +// below reaches the real pthread_create() through this library's own PLT, so a +// self-patch makes the hook call itself: the recursion exhausts the thread +// stack and the process dies of SIGSEGV without even an hs_err file, because +// the JVM's crash handler needs stack of its own to run. +// +// Recognising ourselves by mapped address range is the whole check. Comparing +// realpath(lib->name()) with our own path used to be, and it silently fails: +// dd-trace-java extracts libjavaProfiler.so to a temporary file and unlinks it +// once loaded, so realpath() on the still-mapped path returns nullptr and the +// comparison reported "not self". Whether that mattered depended on a library +// re-scan landing after the unlink, which is why the crashes looked random. +// +// Every native library cache carries its mapping bounds (Symbols::parseLibraries +// builds them from /proc/self/maps), so the range test needs no fallback. +bool LibraryPatcher::is_profiler_library(CodeCache* lib) { + return lib != nullptr && lib->contains(self_anchor()); +} + class RoutineInfo { private: func_start_routine _routine; @@ -406,8 +427,9 @@ static int pthread_create_hook(pthread_t* thread, } void LibraryPatcher::patch_libraries() { - // LibraryPatcher has yet initialized, only happens in Gtest - if (_profiler_name == nullptr) { + // 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.load(std::memory_order_acquire)) { return; } @@ -419,10 +441,7 @@ void LibraryPatcher::patch_libraries() { void LibraryPatcher::patch_library_unlocked(CodeCache* lib) { if (lib->name() == nullptr) return; - char path[PATH_MAX]; - char* resolved_path = realpath(lib->name(), path); - if (resolved_path != nullptr && // filter out virtual file, e.g. [vdso], etc. - strcmp(resolved_path, _profiler_name) == 0) { // Don't patch self + if (is_profiler_library(lib)) { // Don't patch self return; } @@ -489,12 +508,10 @@ void LibraryPatcher::patch_pthread_create() { // (like wasmtime) that install broken signal handlers calling malloc(). void LibraryPatcher::patch_sigaction_in_library(CodeCache* lib) { if (lib->name() == nullptr) return; - if (_profiler_name == nullptr) return; // Not initialized yet + if (!_initialized.load(std::memory_order_acquire)) return; // Not initialized yet // Don't patch ourselves - char path[PATH_MAX]; - char* resolved_path = realpath(lib->name(), path); - if (resolved_path != nullptr && strcmp(resolved_path, _profiler_name) == 0) { + if (is_profiler_library(lib)) { return; } @@ -598,20 +615,7 @@ bool LibraryPatcher::patch_socket_functions() { const CodeCacheArray& native_libs = Libraries::instance()->native_libs(); int num_of_libs = native_libs.count(); - // Pre-resolve all library paths before acquiring the lock: realpath() may - // block on I/O and must not be called while holding _lock. - // We only need the is-self flag per library, so avoid a huge stack allocation. - static_assert(MAX_NATIVE_LIBS > 0, "MAX_NATIVE_LIBS must be positive"); - bool is_self[MAX_NATIVE_LIBS]; int capped = (num_of_libs <= MAX_NATIVE_LIBS) ? num_of_libs : MAX_NATIVE_LIBS; - for (int index = 0; index < capped; index++) { - CodeCache* lib = native_libs.at(index); - is_self[index] = false; - if (lib == nullptr || lib->name() == nullptr) continue; - char path[PATH_MAX]; - char* rp = realpath(lib->name(), path); - is_self[index] = (rp != nullptr && strcmp(rp, _profiler_name) == 0); - } ExclusiveLockGuard locker(&_lock); // Re-check under the lock only on re-entry (when hooks are already installed): @@ -651,7 +655,10 @@ bool LibraryPatcher::patch_socket_functions() { if (lib == nullptr) continue; if (lib->name() == nullptr) continue; - if (is_self[index]) { + // Checked here rather than in a pre-pass keyed by index: the library array + // can grow between the two, and a flag applied to the wrong entry could let + // us patch ourselves. + if (is_profiler_library(lib)) { continue; } diff --git a/ddprof-lib/src/main/cpp/profiler.cpp b/ddprof-lib/src/main/cpp/profiler.cpp index c45a61c523..ca3464e830 100644 --- a/ddprof-lib/src/main/cpp/profiler.cpp +++ b/ddprof-lib/src/main/cpp/profiler.cpp @@ -1573,7 +1573,8 @@ Error Profiler::start(Arguments &args, bool reset) { // Prepare JVMSupport for execution JVMSupport::initExecution(args, VM::jvmti(), VM::jni()); - + // Must precede the first updateSymbols(): it is what allows LibraryPatcher to + // start patching, and the hooks it installs assume a running profiler. LibraryPatcher::initialize(); // Kernel symbols are useful only for perf_events without --all-user diff --git a/ddprof-lib/src/test/cpp/libraryPatcher_ut.cpp b/ddprof-lib/src/test/cpp/libraryPatcher_ut.cpp new file mode 100644 index 0000000000..82bb2d92bc --- /dev/null +++ b/ddprof-lib/src/test/cpp/libraryPatcher_ut.cpp @@ -0,0 +1,170 @@ +/* + * Copyright 2026, Datadog, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +#include + +#if defined(__linux__) + +#include "codeCache.h" +#include "libraryPatcher.h" + +#include + +// LibraryPatcherTestAccessor — friend of LibraryPatcher, exposes the internals +// the self-identification tests need. +class LibraryPatcherTestAccessor { +public: + static const void* selfAnchor() { return LibraryPatcher::self_anchor(); } + + static bool isProfilerLibrary(CodeCache* lib) { + return LibraryPatcher::is_profiler_library(lib); + } + + static void patchLibraryUnlocked(CodeCache* lib) { + LibraryPatcher::patch_library_unlocked(lib); + } + + static int patchedCount() { return LibraryPatcher::_size; } + + // Drops the patch table without writing the saved originals back, unlike + // unpatch_libraries(). Tests patch GOT slots that live in the test body's + // frame, so by the time a fixture could restore them the addresses are dead; + // forgetting the entries is what keeps one test from reaching into the next. + static void forgetPatchTable() { LibraryPatcher::_size = 0; } +}; + +namespace { + +// Stands in for the profiler's own library file after dd-trace-java's +// LibraryLoader extracts it and unlinks it: the mapping is still live, but the +// path no longer resolves, so realpath() returns nullptr. +const char* const kUnlinkedPath = + "/tmp/ddprof_bits/pid_1/scratch/libjavaProfiler-dd-tmp-unlinked.so"; + +// An address range that cannot overlap any real mapping of this binary. +const void* const kForeignMin = (const void*)0x1000; +const void* const kForeignMax = (const void*)0x2000; + +// Stands in for the real pthread_create that a library's GOT slot holds before +// the profiler patches it. +void* fake_original_pthread_create(void* arg) { return arg; } + +const void* anchorMin() { + return (const char*)LibraryPatcherTestAccessor::selfAnchor() - 0x1000; +} + +const void* anchorMax() { + return (const char*)LibraryPatcherTestAccessor::selfAnchor() + 0x1000; +} + +} // namespace + +// The regression test for the self-patching crash: a cache that covers our own +// code must be recognised as ours even when its path cannot be resolved. +// +// Before the fix the only self-check was realpath(name) == profiler path. The +// profiler's library is extracted to a temp file and unlinked once loaded, so +// realpath() returned nullptr, the check reported "not self", and the profiler +// patched its own pthread_create GOT slot. pthread_create_hook() calls +// pthread_create() through that same slot, so it then called itself until the +// thread stack was exhausted — SIGSEGV, and no hs_err, since reporting a crash +// also needs stack. +TEST(LibraryPatcherSelfCheck, RecognisesSelfWhenItsLibraryFileWasUnlinked) { + CodeCache lib(kUnlinkedPath, -1, anchorMin(), anchorMax()); + + EXPECT_TRUE(LibraryPatcherTestAccessor::isProfilerLibrary(&lib)); +} + +// The address range is what identifies us, not the mere fact that a path failed +// to resolve: a library that is genuinely not ours stays patchable even if its +// own path is unresolvable (deleted or replaced on upgrade, say). +TEST(LibraryPatcherSelfCheck, DoesNotRecogniseForeignLibraryWithUnresolvablePath) { + CodeCache lib(kUnlinkedPath, -1, kForeignMin, kForeignMax); + + EXPECT_FALSE(LibraryPatcherTestAccessor::isProfilerLibrary(&lib)); +} + +// Identity is decided by the mapping alone, so a library that is not ours stays +// patchable whatever its path says — including a perfectly resolvable one. This +// pins the contract against a future reintroduction of name matching, which is +// what failed here in the first place. +TEST(LibraryPatcherSelfCheck, DoesNotRecogniseUnrelatedResolvablePath) { + CodeCache lib("/proc/self/cmdline", -1, kForeignMin, kForeignMax); + + EXPECT_FALSE(LibraryPatcherTestAccessor::isProfilerLibrary(&lib)); +} + +// A cache with no known address range (min/max default to +// NO_MIN_ADDRESS/NO_MAX_ADDRESS, for which contains() is always false) is not +// us. Naming our own library must not change that, now that the name plays no +// part in the decision. +TEST(LibraryPatcherSelfCheck, DoesNotRecogniseCacheWithUnknownAddressRange) { + Dl_info info; + ASSERT_NE(0, dladdr(LibraryPatcherTestAccessor::selfAnchor(), &info)); + + CodeCache lib(info.dli_fname); + + EXPECT_FALSE(LibraryPatcherTestAccessor::isProfilerLibrary(&lib)); +} + +// The predicate dereferences nothing but the cache's bounds, so it is safe to +// call at any point — including before the profiler has started, which is when +// the patch sites used to compare against a not-yet-resolved profiler path. +TEST(LibraryPatcherSelfCheck, TreatsNullLibraryAsForeign) { + EXPECT_FALSE(LibraryPatcherTestAccessor::isProfilerLibrary(nullptr)); +} + +// patch_library_unlocked() skips a cache it has already patched by comparing +// CodeCache addresses, and successive tests tend to place their cache at the +// same stack address. Start each test from an empty table so one cannot mask a +// failure in the next. +class LibraryPatcherPatching : public ::testing::Test { +protected: + void SetUp() override { LibraryPatcherTestAccessor::forgetPatchTable(); } +}; + +// End to end through the patcher: given a cache that looks like the profiler's +// own unlinked library and does expose a pthread_create import, the slot must be +// left alone and nothing recorded in the patch table. +TEST_F(LibraryPatcherPatching, LeavesItsOwnPthreadCreateSlotUntouched) { + void* const original = (void*)&fake_original_pthread_create; + void* slot = original; + + CodeCache lib(kUnlinkedPath, -1, anchorMin(), anchorMax(), nullptr, + /* imports_patchable */ true); + lib.addImport(&slot, "pthread_create"); + ASSERT_EQ(&slot, lib.findImport(im_pthread_create)); + + int before = LibraryPatcherTestAccessor::patchedCount(); + LibraryPatcherTestAccessor::patchLibraryUnlocked(&lib); + + EXPECT_EQ(original, slot); + EXPECT_EQ(before, LibraryPatcherTestAccessor::patchedCount()); +} + +// The counterpart, so the test above cannot pass by the patcher simply having +// stopped working: an identical cache that is not ours does get its slot +// redirected to the hook, and unpatching puts the original back. +TEST_F(LibraryPatcherPatching, StillPatchesForeignPthreadCreateSlot) { + void* const original = (void*)&fake_original_pthread_create; + void* slot = original; + + CodeCache lib(kUnlinkedPath, -1, kForeignMin, kForeignMax, nullptr, + /* imports_patchable */ true); + lib.addImport(&slot, "pthread_create"); + + int before = LibraryPatcherTestAccessor::patchedCount(); + LibraryPatcherTestAccessor::patchLibraryUnlocked(&lib); + + EXPECT_NE(original, slot); + EXPECT_EQ(before + 1, LibraryPatcherTestAccessor::patchedCount()); + + LibraryPatcher::unpatch_libraries(); + + EXPECT_EQ(original, slot); + EXPECT_EQ(0, LibraryPatcherTestAccessor::patchedCount()); +} + +#endif // __linux__