Repository navigation
Write saved GOT slots only while their library is loaded - #838
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3743d83d15
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
🔗 Commit SHA: d8eeb61 | Docs | View more details | Give us feedback! |
There was a problem hiding this comment.
The liveness predicate can still recognize a stale CodeCache after a same-file reload at the same base, allowing writes into a newly protected GOT. Deleted-file path matching also aliases replacements and rejects live relative-path loads.
🤖 Bits Code Review · Commit 3743d83 · @DataDog review to ask questions
3743d83 to
660ef43
Compare
CI Test ResultsRun: #37459854565 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-10-06 17:46:45 UTC |
There was a problem hiding this comment.
For unlinked DSOs, the new fallback equates path with identity: a replacement DSO can inherit a stale cache at the same base, while a live DSO loaded through a relative path or symlink can be missed. Those cases can corrupt GOT contents or leave hooks installed.
🤖 Bits Code Review · Commit 660ef43 · @DataDog review to ask questions
660ef43 to
d1bb979
Compare
LibraryPatcher records the GOT slot addresses it patches for socket I/O, pthread_create and sigaction, and the library list never drops a CodeCache. Nothing pinned the patched libraries, so after a library was dlclose()d and unmapped, restoring the hooks on stop, or re-installing them on the next start, wrote through the stale address: a SIGSEGV if the range was unmapped, silent corruption if another mapping reused it. OpenJDK unloads a JNI library when its class loader is collected, so this is reachable from application code. Route every such write through visit_live_libraries(), which makes it from inside a dl_iterate_phdr() callback. glibc's dlclose() unmaps and unlinks an object while holding dl_load_write_lock, the lock the iteration holds throughout, so a library reported to the callback stays mapped until the callback returns. A CodeCache is matched to a loaded object by image base and by the dev/inode of the backing file, recorded from /proc/self/maps as CodeCache::fileId(). When the file was unlinked after loading, as netty's native loader does by default, the match falls back to the path. Entries of unloaded libraries are dropped without being written. musl's dlclose() never unmaps, so musl skips the check. Add libraryPatcherUnload_ut, which loads a small C library importing write() and pthread_create(), patches it, unloads it and then stops or restarts patching. It covers both the unmapped and the reused-address case, a different file at the same base, unlinked library files, and a still-loaded library being restored. StillPatchesForeignPthreadCreateSlot now expects a slot whose library is not loaded to be left alone on unpatch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A library unloaded and reloaded from the same file at the same address still matches its old CodeCache by base and inode, which is intended: the reload is not re-parsed, and the saved slots are valid for the identical layout. But the cache remembered that its GOT had been made writable, while under full RELRO the new mapping's GOT is read-only, so the next restore or re-patch faulted. Make the imports writable again for every library confirmed loaded, before visiting it. Tighten the fallback used when the backing file was unlinked after loading. A relatively loaded library keeps its relative name in the loader while /proc/self/maps has the absolute path, so match a relative name as a trailing part of the path. And require the program header table to match a hash recorded at parse time, so that a different library loaded from the same path, unlinked too and mapped at the same base, is not taken for the old one. Build the test library with full RELRO and test reloads at the same base for stop and restart, a relatively loaded unlinked library, and an unlinked cache with a different layout. Add the copyright header to the test library's Makefile. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the name a library was loaded by no longer resolves to its file, because the file was unlinked or the name is relative or a symlink, the liveness check fell back to comparing that name with the path from /proc/self/maps. That breaks for symlinks and for relative names that resolve differently, and a path says nothing about which file was loaded from it. Replace the path comparison with an image fingerprint: a hash of the program header table and of the PT_NOTE segments, which carry the GNU build-id, recorded at parse time and recomputed from dl_phdr_info inside the dl_iterate_phdr() callback. A loaded object is now the cached library if it has the same base and either the same backing file or the same fingerprint. Matching the fingerprint means the same build at the same address, whose GOT slots are where the cache expects them. Add tests for a library loaded through a symlink, for each identity on its own, and for two builds that differ only in their build-id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
musl's dlclose() never unmaps, so the patcher visits every candidate there without checking its identity, and a test that expects a stale cache to be rejected cannot pass. The other rejection tests already skip on musl through the check that dlclose() can unload the test library. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a9f13c2 to
1340a26
Compare
The liveness check accepted a cached library when either its backing file or its image fingerprint matched the loaded object at the same base. The file does not identify the image: a different build can be written to a file that reuses the freed inode of an unloaded library, and is then not parsed at all, because its dev/inode has been seen before. A relative or symlinked name may also resolve to another file by the time of the check. In both cases the stale CodeCache was taken for the loaded object and its saved GOT slots were written into it. Let the fingerprint decide whenever the cache has one, and fall back to the backing file only for caches without a fingerprint. The file is now stat()ed lazily, only in that case. PatchAcceptsCacheOfSameFileAtSameBase becomes PatchSkipsCacheOfSameFileButDifferentImage, and two tests cover the fallback for a cache without a fingerprint. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
State why add_live_candidate() cannot run out of room, and assert it, instead of silently dropping a candidate: every caller adds at most one per CodeCache, and a library's socket entries form a single run. Narrow the visit_live_libraries() contract to LibraryPatcher's own writes and name the exception: MallocHooker patches GOT slots while holding an UnloadProtection on the library and never restores them. In the tests, describe the unlinked-library case by the fingerprint rather than by path, which it no longer uses, give FingerprintCoversBuildId its own exit code, remove the temporary library copy when a child bails out early, drop the ticket references from comments, and use the SPDX copyright header. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Reviewed revision [P1] Do not use a headers-only fingerprint as image identityLocation: symbols_linux.cpp:1293–1307, accepted by libraryPatcher_linux.cpp:123–128.
I reproduced this in an isolated Linux/aarch64 glibc probe using the exact The final step applied the restoration that this predicate permits, after making the page writable. It corrupted the replacement library's read slot. This leaves the address-reuse case in PROF-16135 unresolved for libraries without build IDs. Please require stronger image/load identity and add a regression test with two actual replacement DSOs without build IDs; tests with artificially different fingerprints do not exercise this case. [P1] Avoid acquiring the loader lock while holding the patch lockLocation: libraryPatcher_linux.cpp:70–83. The new walk runs while With concurrent patching, the lock cycle is:
I verified this lock cycle in a reduced two-thread Linux probe using the repository's actual Validation scope: these were isolated Linux probes, not a run of the full profiler test suite. The fingerprint probe demonstrated the replacement-slot corruption directly; the deadlock finding combines source inspection of the JVM hook/refresh path with a reduced reproduction of the lock cycle. |
The fingerprint that identifies a loaded library hashed its program header table and its PT_NOTE segments. Without a build-id that is just the segment layout, so two different libraries with identical program headers, such as two builds that differ only in importing write() or read(), got the same fingerprint. Loaded at the same base, the stale CodeCache of one was taken for the other, and stopping restored the saved write() binding into the GOT slot that holds read() in the replacement. Also hash every dynamic relocation that names a symbol, from DT_JMPREL and DT_RELA/DT_REL: its offset, type and symbol index, and the symbol's name. Equal fingerprints at the same base now mean that each saved slot address holds the same import. The tables are read only within the file-backed parts of the PT_LOAD segments, which replaces the explicit readable range: at parse time the library is held by UnloadProtection, as parseDynamicSection() already relies on. Add two builds without a build-id that differ only in importing write() or read(), a test that their fingerprints differ, and a test that replaces the patched library with the other build at the same base and checks that stopping leaves the replacement's slot alone. Link the test libraries with the host page size as max-page-size: with aarch64's default 64K alignment glibc over-allocates the mapping, a reload never lands at the old base, and the reload tests always skipped there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Patching held _lock while dl_iterate_phdr() waited for glibc's dl_load_write_lock. But dl_iterate_phdr() runs arbitrary callbacks under that lock, and a callback that loads a library through the JVM, a JNI System.loadLibrary() for instance, reaches Profiler::dlopen_hook() and a synchronous refresh that waits for _lock. With a concurrent stop or refresh holding _lock, each thread waited for the other forever. Make dl_load_write_lock, then _lock, the only order. Patching and unpatching now run in a LiveLibraryWalk, which takes _lock in the first dl_iterate_phdr() callback, collects its candidates there and holds the lock until it is destroyed. A refresh from inside a foreign callback re-enters the recursive dl_load_write_lock and then waits for _lock, whose holder waits for nothing. In turn, code under _lock must not wait for dl_load_lock or dl_load_tls_lock, which a concurrent dlopen() or dlclose() holds while waiting for dl_load_write_lock: none of it calls dlopen(), dlsym(), dladdr() or dlinfo(), and the dlsym() calls of patch_socket_functions() stay outside the walk. musl skips the walk and just takes _lock. Add a test that patches from inside a foreign dl_iterate_phdr() callback while another thread stops socket patching. It deadlocks, and fails on an alarm, with the old order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Both findings confirmed and fixed: [P1] Headers-only fingerprint — fixed in 92dc8f2. The fingerprint now also covers the import layout: every dynamic relocation that names a symbol ( [P1] Lock order — fixed in d8eeb61. The only order is now Verified on linux-aarch64 glibc 2.35 (container): all 75 |
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/142692549 |
Fixes PROF-16135. Builds on #837 (merged), which builds this PR's test DSO.
What does this PR do?:
Makes
LibraryPatcherwrite a saved GOT slot only while the library that owns it is provably still loaded. That covers socket I/O hooks,pthread_createandsigaction, on both stop and start.Motivation:
LibraryPatcherstores raw GOT-slot addresses, andCodeCacheArraynever drops aCodeCache. Nothing pinned the patched libraries. Once one wasdlclose()d and unmapped:unpatch_socket_functions,unpatch_libraries);patch_socket_functions,patch_pthread_create). So an unload while the profiler was stopped crashed the next start.The result is a SIGSEGV if the range is unmapped, and silent corruption if another mapping reused it. The ticket only described the socket-unpatch case; the restart path and
pthread_create(which isn't gated on natsock) have the same bug. The old in-code justification ("the host JVM does not dlclose libc-importing DSOs") is false: OpenJDK unloads a JNI library when its class loader is collected (NativeLibraries→JVM_UnloadLibrary→os::dll_unload→dlclose). The realistic trigger is a plain-C JNI library on glibc, e.g. on app-server redeploys. C++ libraries withSTB_GNU_UNIQUEsymbols, and anything on musl, never actually unmap.The change:
All three patchers now go through
visit_live_libraries(), which makes each write inside adl_iterate_phdr()callback:_dl_close_workerunmaps (DL_UNMAP) and unlinks an object while holdingdl_load_write_lock, anddl_iterate_phdrholds that lock for the whole walk. So an object reported to the callback cannot be unmapped until the callback returns. Verified inelf/dl-close.candelf/dl-iteratephdr.cfor glibc 2.17, 2.28, 2.35 and current master.CodeCachematches a loaded object only with the same image base and the same image fingerprint. The backing file is used only for a cache without a fingerprint.DT_JMPRELandDT_RELA/DT_REL(offset, type, symbol index and the symbol's name). It's recorded at parse time (CodeCache::imageFingerprint()) and recomputed fromdl_phdr_infoin the callback. Unlike the file, it still identifies the library when the loader's name no longer resolves to it, because the file was unlinked after loading, or the name is relative or a symlink. This matters: netty'sNativeLibraryLoaderdeletes extracted libraries by default (io.netty.native.deleteLibAfterLoading=true).stat(dlpi_name)dev/inode vs the newCodeCache::fileId(), recorded from/proc/self/maps, and only for a cache without a fingerprint. It doesn't identify the image: a different build can be written to a file that reuses a freed inode (and is then never parsed, because that dev/inode has been seen), and a relative or symlinked name can come to point at another file. (Checked on a real process: inode matching holds across merged-/usrpaths and versioned symlinks, where plain name comparison doesn't.) In practice every cache with imports has a fingerprint, since both are set inparseProgramHeaders().write()orread()have identical program headers, and only the import layout tells them apart.CodeCache(the reload isn't re-parsed, since its inode has been seen). Its saved slots stay valid, but under full RELRO the new mapping's GOT is read-only again. So the imports are made writable again before every visit, instead of trusting the cache's earlier state.dlclose()never unmaps.Additional Notes:
Please push back on these:
dl_load_write_lock, then_lock. Patching and unpatching run in aLiveLibraryWalk, which takes the patch lock (_lock) in the firstdl_iterate_phdrcallback, collects its candidates there and holds the lock until it is destroyed.dl_iterate_phdrruns arbitrary callbacks underdl_load_write_lock. One that loads a library through the JVM (a JNISystem.loadLibrary()) reachesProfiler::dlopen_hook()and a synchronous refresh that patches. Had a concurrent stop or refresh held_lockwhile waiting for the write lock, the two would wait for each other forever. In this order the refresh re-enters the recursive write lock and waits for_lock, whose holder waits for nothing._lockmust not wait fordl_load_lockordl_load_tls_lock, which a concurrentdlopen/dlcloseholds while waiting for the write lock. Nothing under it callsdlopen,dlsym,dladdrordlinfo(thedlsymcalls ofpatch_socket_functions()stay outside the walk). It doesmprotect,stat,TEST_LOG/Log::warn(vsnprintf/fprintf) and relaxed atomic counter updates, and touches no dynamic TLS._lock.dlmopen) are no longer patched, becausedl_iterate_phdrreports only the caller's namespace.UnloadProtectioncan'tdlopenit by the absolute path). It was never patched before this PR either.has_entry_for(pthread_create, sigaction) and the slot-location check (sockets) treat it as already patched. It runs unhooked until the next stop/start; for sigaction, whose entries are never cleared, for the rest of the process.StillPatchesForeignPthreadCreateSlotused to expect a fake, never-loadedCodeCache's slot to be written back on unpatch. It now asserts the entry is dropped without a write, which is the point of the fix. Restoring a real loaded library is covered byUnpatchRestoresLoadedLibrary/PthreadCreate.dl_iterate_phdrto take the locks in order. With no candidates (prefilters keep only libraries that still need work) it stops after the first object. Once a library has been unloaded, though, its stale cache stays a candidate, so from then on every patch pass walks all objects, briefly holdingdl_load_write_lockand blocking other threads' exception unwinding. Per object at a candidate's base it hashes the program headers, notes and symbol-naming relocations once (leading relative relocations are skipped viaDT_RELACOUNT);statruns only for a cache without a fingerprint. No allocation: candidates live in a static array guarded by_lock, sized for one candidate perCodeCache(asserted).A
dlclosehook was considered and rejected. It can't see unloads while the profiler is stopped (the trap is disabled), native code callingdlclosedirectly, or dependencies unloaded along with the closed library. And with this check in place, it isn't needed.How to test the change?:
libraryPatcherUnload_ut.cpp(new, 26 test cases) uses small plain-C DSOs (native-libs/patch-target-lib): one importingwrite()andpthread_create(), two that differ only in build-id, and two without a build-id that differ only in importingwrite()orread(). Each scenario runs in a forked child, because patching is process-global and the bug crashes.UnpatchDoesNotWriteIntoReusedMappingUnpatchDoesNotFaultOnUnmappedLibraryRepatchDoesNotWriteIntoReusedMappingRepatchAfterReloadAtSameBase/UnpatchAfterReloadAtSameBaseUnpatchRestoresLoadedLibraryPatchSkipsCacheOfDifferentImageAtSameBase,PatchSkipsCacheOfSameFileButDifferentImage,PatchAcceptsCacheOfSameImageAtSameBase,Patch{Accepts,Skips}CacheWithoutFingerprintOf{Same,Different}File…SameFileButDifferentImage(inode reuse) wrote the stale cache's slotPatchesLibraryUnlinked{Before,After}Parsing,…RelativePathAndUnlinked,…SymlinkAndUnlinked,PatchSkipsUnlinkedCacheOfDifferentImageFingerprintCoversBuildIdFingerprintCoversImportswrite()/read()got the same fingerprintUnpatchDoesNotWriteIntoReplacementWithSameLayoutwrite()build is replaced by theread()build at the same base, stop restoredwrite()into the slot holdingread()PatchInsideForeignWalkDoesNotDeadlockWithStopdl_iterate_phdrcallback while another thread stops: deadlock (fails on a 10 s alarm)The test DSOs are built with full RELRO (
-z now), as on hardened distros, and with the build host's page size asmax-page-size: with aarch64's default 64K segment alignment glibc over-allocates the mapping, so a reload never lands at the old base and the reload tests always skipped there.PatchSkipsCacheOfSameFileButDifferentImage;FingerprintCoversBuildId;FingerprintCoversImportsandUnpatchDoesNotWriteIntoReplacementWithSameLayout;_locktaken before the walk):PatchInsideForeignWalkDoesNotDeadlockWithStop;dlclosecan unmap at all is checked before the profiler sees the DSO, and they skip only if it can't (musl).gtestDebugtests pass. Run CI-style (buildGtest{Release,Asan}, then every binary directly), 75 release and 74 ASan binaries pass.testDebugpasses 225/225 (19 environment skips; allNativeSocket*tests ran).gtestDebugbinaries pass;libraryPatcherUnload_ut26/26 with no skips in both debug and ASan;libraryPatcher_utpasses under ASan.testDebug -Ptests="*NativeSocket*": all pass, includingNativeSocketRestartTest(NativeSocketMacOsNoOpTestskips off macOS).For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.🤖 Generated with Claude Code