refactor(got-patching): extract shared GOT-patching primitives into libdd-got-hook - #2282
refactor(got-patching): extract shared GOT-patching primitives into libdd-got-hook#2282gyuheon0h wants to merge 6 commits into
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results📦
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 67df7e6 | Docs | Datadog PR Page | Give us feedback! |
63b2fca to
60c1e60
Compare
gyuheon0h
changed the title
gyuheon0h
changed the title
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
Move the ELF parsing, dl_iterate_phdr iteration, PageProtGuard, gnu_hash_symbol_count, gnu_hash_lookup, lookup_symbol, and hook_symbol utilities out of libdd-profiling-heap-gotter into a new libdd-got-hook crate. libdd-profiling-heap-gotter now depends on libdd-got-hook and keeps only the SymbolOverrides multi-symbol registry and per-library dedup/rescan logic. Also adds DT_HASH (sysv) fallback for determining dynsym entry count, so objects linked with --hash-style=sysv are no longer silently skipped.
822405c to
6ab7768
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 822405c2e5
ℹ️ 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".
| let sym_idx = elf64_r_sym(reloc.r_info) as u32; | ||
| if let Some(cstr) = dyn_info.sym_name(sym_idx) { | ||
| if cstr.to_bytes() == symbol_name { | ||
| let addr = reloc.r_offset as usize + dyn_info.base_address; | ||
| if guard.override_entry(addr, hook_fn) { |
There was a problem hiding this comment.
When a loaded object has a symbol-matching dynamic relocation that is not a GOT pointer relocation—for example a legacy R_X86_64_PC32 text relocation—this loop still treats r_offset as an eight-byte pointer slot. override_entry then overwrites eight bytes even though that relocation's field is only four bytes and has different semantics, corrupting adjacent code or data. Inspect the architecture-specific relocation type from r_info and patch only supported pointer-slot types such as GLOB_DAT and JUMP_SLOT (or handle every accepted type according to its width and addend semantics).
Useful? React with 👍 / 👎.
| let mut patched_any = false; | ||
| let mut guard = PageProtGuard::new(); | ||
|
|
||
| let guard_ptr = &mut guard as *mut PageProtGuard; | ||
| let patched_ptr = &mut patched_any as *mut bool; | ||
|
|
||
| iterate_libraries(|info, _is_exe| { |
There was a problem hiding this comment.
When two threads invoke hook_symbol concurrently—or this function overlaps with the heap gotter's patching pass—each creates an independent PageProtGuard and changes the same GOT page protections without synchronization. One guard can restore the page to read-only while the other is still writing, causing a fault, while a guard that observed the temporary RW mapping can instead restore RW and permanently weaken RELRO. Serialize all patching passes with a process-wide lock, or make non-overlap an explicit enforced safety precondition.
Useful? React with 👍 / 👎.
6ab7768 to
285cdd3
Compare
BenchmarksComparisonBenchmark execution time: 2026-07-28 03:08:31 Comparing candidate commit 67df7e6 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 141 metrics, 0 unstable metrics.
|
978bf00 to
67df7e6
Compare
1 participant
What does this PR do?
The crashtracker needs to hook
__assert_failat runtime using GOT patching. Rather than duplicating ~600 lines of ELF parsing and page-protection code, this extracts the shared primitives into a crate that bothlibdd-profiling-heap-gotterandlibdd-crashtrackercan depend on.What moves to
libdd-got-hookElf64_Dyn,DT_*)DynamicInfo::from_phdr-- parsePT_DYNAMICfrom a loaded ELF objectdl_iterate_phdrwrapper with panic-safe trampolineRELRO-aware page protection managementlookup_symbolelf64_r_sym/check_sym— small helpershook_symbol. This one is new. It is a one-shot single-symbol GOT patcher that wiresdlsym,iterate_libraries, andpatch_got_entriesinto a single callWhat stays in libdd-profiling-heap-gotter
SymbolOverrides; the multi-symbol registryhooks.rs; the actualmalloc/free/calloc/reallochook functionsinstall_heap_overrides/update_heap_overridespublic APIWhat "changed"
Nonewhengnu_hash.is_null()but now it falls through tosysv DT_HASH nchain, then thesymtab/strtabdistance heuristic. This means objects linked with--hash-style=sysvare now parsed instead of skipped. Forheap-gotterthis is strictly additive more libraries get patched, none get un-patched. This was also a codex recommendation and I am not sure that this is necessary but from what I understand, this does not change behavior for heap prof usage.patch_got_entriesandprocess_relocationnow skip relocations that arentGLOB_DATorJUMP_SLOT. Before, theyd patch any relocation matching symbol name. In practice, allmalloc,free,calloc,reallocshould beGLOB_DAT/JUMP_SLOT.Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.