Repository navigation
PROF-15982 Skip in-memory ELF parse of a library musl is still mapping - #996
zhengyu123 wants to merge 7 commits into
Conversation
On musl, UnloadProtection accepted every library, so Symbols::parseLibraries() read the dynamic section of a library that another thread's dlopen() had mapped only partly. musl maps the whole span from file offset 0 first and the later segments afterwards, so those reads saw the wrong bytes or faulted. On musl the guard now requires dladdr() to find a library that starts at the image base. musl's dladdr() searches only libraries whose mapping is done. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… leaks Map the copy read-only, honour TMPDIR, treat read errors as copy failures and remove the copy on every path. State on the musl branch that it checks load completion, as dlopen() does on glibc. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…test Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
More details
Static review supports the musl guard’s use of loader registration and image-base equality before allowing in-memory ELF parsing. The concurrent dlopen race itself was not reproduced.
🤖 Bits Code Review · Commit 01b2a3d
Scan-Build Report
Bug Summary
Filter Results by FileReports
|
|||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01b2a3d5dc
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| int out = mkstemps(to, 3); | ||
| if (out < 0) return false; | ||
| int in = open(from, O_RDONLY); |
There was a problem hiding this comment.
The new test manually pairs mkstemps/close and unlink, plus open/close and mmap/munmap, rather than using owner guards. This violates the repository’s C++ RAII rule; use scoped owners for these resources or explain why a manual pair cannot be scoped.
AGENTS.md reference: AGENTS.md:L469-L474
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 686846a. Three scoped owners in the test now release every resource on every exit path: FdOwner (fd), FileMapping (mmap/munmap) and TempCopy (mkstemps copy, unlinked in its destructor). The ASSERTs run inline again. isLoaded() closes the dlopen(RTLD_NOLOAD) handle right where it opens it.
…guards Scoped owners release the temp copy, descriptors and mapping on every exit, so the ASSERTs run inline instead of after hand-ordered cleanup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rrno copyToTempFile() in testTempFile.h replaces the copies in unloadProtection_ut and libraryPatcherUnload_ut. It honours TMPDIR, returns the errno of the failing step (EIO for a short write) and removes a partial copy. The tests print that errno and the failing step instead of a later errno. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rrors The helper takes the directory; a null one means TMPDIR, or /tmp when unset. libraryPatcherUnload_ut dlopen()s its copy, so it keeps /tmp rather than a TMPDIR that may be noexec, and prints the copy error before exiting. The helper returns ENAMETOOLONG instead of truncating the path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CI Test ResultsRun: #43547292494 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-11-14 03:00:56 UTC |
|
What does this PR do?:
On musl,
UnloadProtectionnow passes only for a library the dynamic loader has finished mapping.Symbols::parseLibraries()then skips the in-memory ELF parse (ElfParser::parseProgramHeaders()) of a library that another thread'sdlopen()is still loading.UnloadProtectionon musl:_validrequiresdladdr(image_base)to return a library whosedli_fbaseequalsimage_base. The main executable and the loader are still accepted without a check.unloadProtection_ut.cpp: rejects a file mapping the loader does not know, accepts a loaded image, and rejects an address inside a loaded library that is not its base.testTempFile.h:FdOwnerandcopyToTempFile(), shared byunloadProtection_utandlibraryPatcherUnload_ut(whose own copy loop it replaces).Motivation:
PROF-15982: SIGSEGV in
ElfParser::parseDynamicSection()duringProfiler::start()on musl aarch64 (tracer 1.66.0, which bundles ddprof 1.50.0).linux-arm64-musllibrary,parseDynamicSection()+0x158isldr x16, [x8]: thed_tagread in the first line of the dynamic-section loop. At the fault,x8points insidelibverify.soandx9is the dynamic-section end. ThePT_DYNAMICsegment itself was unreadable.map_library()first maps the whole address span from file offset 0, then maps each later segment over it withMAP_FIXED. It links the library into its loaded list only after that. In between,/proc/self/mapsalready lists the library, but its later segments hold the wrong file bytes or SIGBUS past EOF.UnloadProtection'sdlopen(RTLD_NOLOAD)waits for a load in progress. On musl the class returned valid without any check, because musl never unloads.Additional Notes:
dladdr(), notdlopen(RTLD_NOLOAD)on musl. musl'sdladdr()searches only libraries already linked into its list, that is, fully mapped. musl looks up an absolute path by opening the file, soRTLD_NOLOADwould reject libraries whose file was deleted after loading. Those keep working withdladdr().UnloadProtectioncallers on musl now get the check, including the GOT patching inMallocHookerandapplyPatch(). That matches what the class already guarantees on glibc.ElfParser::parseFile(); only its in-memory imports and unwind tables are missing. glibc behaves the same way whenUnloadProtectionis invalid. Tracked in PROF-16675./code-reviewmedium on each of five fix ranges, including the fix for the Codex RAII comment. No Critical or High finding is open. Open, Low or nit only, all in test code:libraryPatcherUnload_utcopies its DSO to/tmp, so it fails on a host where/tmpis noexec. It used/tmpbefore this PR as well."/tmp"appears three times inlibraryPatcherUnload_utinstead of one named constant.dlopen(),chdir()orsymlink()inloadUnlinkedLib()exits without printing the cause.ENAMETOOLONG,copyToTempFile()leaves the truncated template inpath_out.How to test the change?:
Run on the head commit unless noted; aarch64 containers, debug build,
utils/run-containers-tests.sh --mount.gtestDebug_unloadProtection_uton musl without the fixRejectsImageNotLoadedByTheLoader,RejectsLoadedImageAtWrongBasegtestDebug_unloadProtection_uton musl, 3/3 tests rungtestDebug_unloadProtection_uton glibcgtestDebug_libraryPatcherUnload_uton musl (6 passed, 20 skipped by design on musl) and glibcgtestDebug_elfparser_uton musl and glibcqualityChecks(first fix commit),check-copyright.py,check-claude-md.pyproduction-check)The race itself is not reproduced: the gtest checks the guard's decision on a raw file mapping that the loader never registered.
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