diff --git a/ddprof-lib/src/main/cpp/libraryPatcher.h b/ddprof-lib/src/main/cpp/libraryPatcher.h index 70be3659b..97a3f26e3 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 e458fe6a1..97e75822e 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 c45a61c52..ca3464e83 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 000000000..82bb2d92b --- /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__