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 3 commits into
Conversation
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results📦
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 31e53ca | 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
|
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".
6ab7768 to
285cdd3
Compare
BenchmarksComparisonBenchmark execution time: 2026-07-31 18:22:33 Comparing candidate commit 66e5b36 in PR branch Found 17 performance improvements and 3 performance regressions! Performance is the same for 122 metrics, 0 unstable metrics.
|
978bf00 to
67df7e6
Compare
|
Hey @gyuheon0h cool - this is a good idea! We don't want to end up maintaining N variants of this sort of thing. |
ea56fb3 to
ced5999
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ced5999cd0
ℹ️ 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".
|
Nice - I wanted to do this! Will review first thing Monday 💪 Please don't merge beforehand as I'd like to double check all the allocation profiling stuff that rests on it. |
ced5999 to
e9b992a
Compare
|
@yannham @scottgerring Ill just stack another PR on top of this with the new changes, so that this PR strictly only focuses on the initial "no new feature" refactor 👍 |
560d0a6 to
45c074d
Compare
Move the ELF parsing, dl_iterate_phdr iteration, PageProtGuard, gnu_hash_symbol_count, gnu_hash_lookup, lookup_symbol, and related 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. This is a pure code move — no behavioral changes.
45c074d to
11a3920
Compare
scottgerring
left a comment
scottgerring
left a comment
There was a problem hiding this comment.
Hey @gyuheon0h , thanks for taking the time to split this out and make it re-usable! 🙌
Apart from a handful of nits, I think the two things to agree on are the name of the crate (some suggestions inline) and the shape of the API introduced around DynamicInfo
| /// currently-loaded ELF object. | ||
| pub unsafe fn gnu_hash_lookup(info: &DynamicInfo, name: &[u8]) -> Option<Elf64_Sym> { | ||
| let hashtab = info.gnu_hash; | ||
| if hashtab.is_null() || info.gnu_hash_words < 4 { |
There was a problem hiding this comment.
Extra defensive check welcome!
| } | ||
|
|
||
| /// Access to REL relocations (pointer, count). | ||
| pub fn rels(&self) -> (*const Elf64_Rel, usize) { |
There was a problem hiding this comment.
I'm not sure the accessors here are adding much apart from indirection; this is just pushing the unsafe outwards.
I think something like this would be better as it adapts to a safe API and unburdens the caller, but I am also interested in what @yannham thinks:
pub unsafe fn rels(&self) -> Option<&[Elf64_Rel]>.. basically if we're going to change this in the mechanical refactoring bit, we may as well deal with the null check and slice construction in the API itself.
There was a problem hiding this comment.
Yeah, I agree a slice API would be nice. I guess we can even make it safe if the ELF ABI guarantees that a well-formed Elf file will have an array-like layout for self.rels (and check for nulls here).
aea0f42 to
66e5b36
Compare
66e5b36 to
31e53ca
Compare
| //! | ||
| //! Scope: | ||
| //! * 64-bit Linux ELF only (`Elf64_*`). | ||
| //! * GNU hash tables only (`DT_GNU_HASH`). `DT_HASH` is not parsed; objects without a GNU hash |
There was a problem hiding this comment.
do we have a way of tracking metrics on how often this happens?
| }; | ||
|
|
||
| // ELF dynamic-section tags. The `libc` crate doesn't export these | ||
| // (they're processor-independent ELF spec constants). Values from `<elf.h>`. |
There was a problem hiding this comment.
Do we have any sort of check that they stay in sync?
|
|
||
| ## What it does | ||
|
|
||
| When a shared library calls an external function like `malloc`, it jumps through a pointer in its **Global Offset Table** -- a writable table that the dynamic linker fills at load time. This crate walks every loaded ELF object via `dl_iterate_phdr`, parses its `PT_DYNAMIC` segment, and rewrites GOT entries so calls are redirected to a hook function. The original function address is resolved and returned so the hook can forward to it. |
There was a problem hiding this comment.
Is there good documentation on how ELF / GOT works that we can link here?
There was a problem hiding this comment.
The ancient dark arts passed down from engineer to engineer 😆 😱
I found an article in phrack which is probably not corporate-appropriate but is pretty great. @nsavoire i'm not sure if you've ever seen anything good ?
|
|
||
| static ORIG_FN: AtomicUsize = AtomicUsize::new(0); | ||
|
|
||
| unsafe extern "C" fn my_hook(/* same signature as target */) { |
There was a problem hiding this comment.
Is there a best practice for ensuring this remains in sync / checking this at runtime?
| unsafe { | ||
| hook_symbol(c"__assert_fail", my_hook as *const () as usize, &mut orig_addr); | ||
| } | ||
| ORIG_FN.store(orig_addr, Ordering::Release); |
There was a problem hiding this comment.
If Release is required, document why
| if self.relas.is_null() || self.relas_count == 0 { | ||
| &[] | ||
| } else { | ||
| unsafe { core::slice::from_raw_parts(self.relas, self.relas_count) } |
| if self.jmprels.is_null() || self.jmprels_count == 0 { | ||
| &[] | ||
| } else { | ||
| unsafe { core::slice::from_raw_parts(self.jmprels, self.jmprels_count) } |
| } | ||
|
|
||
| let nbuckets = *hashtab; | ||
| let symbias = *hashtab.add(1); |
There was a problem hiding this comment.
where are these offsets documented
| pub fn new() -> Self { | ||
| // sysconf can return -1 on error; fall back to a conservative | ||
| // 4 KiB default if the query fails. | ||
| let raw = unsafe { sysconf(_SC_PAGESIZE) }; |
| /// Used to remember each GOT page's original protection so we can restore | ||
| /// it after patching, rather than leaving Full-RELRO pages read-write for | ||
| /// the lifetime of the process. | ||
| pub fn read_proc_maps() -> Vec<MapEntry> { |
There was a problem hiding this comment.
are there any tests for this?
scottgerring
left a comment
scottgerring
left a comment
There was a problem hiding this comment.
LGTM! Thanks for addressing comments. Suggest addressing continued feedback on "how we can make this better" in the stacked PR.
4 participants
Stacked under feat(got-hook): add DT_HASH fallback, relocation type guard, and hook_symbol
What does this PR do?
Moves the ELF GOT-patching infrastructure out of libdd-profiling-heap-gotter into a new shared crate libdd-got-hook, so multiple crates can reuse the same machinery for runtime function interposition.
This is a pure code move. I tried to have no behavioral changes here.
What moves to
libdd-got-hookDynamicInfo::from_phdr: parse PT_DYNAMIC from a loaded ELF objectgnu_hash/gnu_hash_symbol_count/gnu_hash_lookup/check_sym: GNU hash table utilitiesiterate_libraries: this is adl_iterate_phdrwrapper with panic-safe trampolinePageProtGuard/read_proc_maps/MapEntry: RELRO-aware page protection managementlookup_symbol/LookupResult: used for resolving a symbol across all loaded objectself64_r_sym: relocation info helperWhat stays in
libdd-profiling-heap-gotterSymbolOverrides: multi-symbol registryhooks.rs: the actualmalloc/free/calloc/reallochook functionsinstall_heap_overrides/update_heap_overridespublic APIMotivation
What inspired you to submit this pull request?
Additional Notes
Gated on @scottgerring's approval
How to test the change?
Describe here in detail how the change can be validated.