feat(got-hook): add DT_HASH fallback, relocation type guard, and hook_symbol - #2297
feat(got-hook): add DT_HASH fallback, relocation type guard, and hook_symbol#2297gyuheon0h wants to merge 2 commits into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results✅ No issues found! 📦
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 788f2de | Docs | Datadog PR Page | Give us feedback! |
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
|
BenchmarksComparisonBenchmark execution time: 2026-07-31 19:39:03 Comparing candidate commit 788f2de in PR branch Found 2 performance improvements and 2 performance regressions! Performance is the same for 138 metrics, 0 unstable metrics.
|
1c3157f to
5f00ce2
Compare
e9b992a to
18d4e84
Compare
1c3157f to
5f00ce2
Compare
18d4e84 to
00baa22
Compare
5f00ce2 to
4618f83
Compare
00baa22 to
d4b9d82
Compare
4618f83 to
8713e2e
Compare
d4b9d82 to
560d0a6
Compare
0f700c2 to
934d93c
Compare
560d0a6 to
45c074d
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: 934d93c530
ℹ️ 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".
| matches!( | ||
| r_type, | ||
| R_X86_64_GLOB_DAT | R_X86_64_JUMP_SLOT | R_AARCH64_GLOB_DAT | R_AARCH64_JUMP_SLOT |
There was a problem hiding this comment.
On 64-bit Linux targets other than x86_64/aarch64, this crate still compiles and the GOT-patching APIs are active, but this shared predicate now admits only the x86_64/aarch64 relocation numbers. Architectures such as s390x, ppc64, or riscv64 use different GLOB_DAT/JUMP_SLOT values, so their import relocations are all skipped and installation can report success after resolving originals without actually patching any GOT entries. Either gate the Linux implementation to the supported architectures or add the architecture-specific constants.
Useful? React with 👍 / 👎.
934d93c to
5e08f80
Compare
45c074d to
11a3920
Compare
5e08f80 to
e2110f9
Compare
e2110f9 to
cad573a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2110f9534
ℹ️ 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".
6264ccb to
d224c5d
Compare
| if !is_got_pointer_reloc(elf64_r_type(reloc.r_info)) { | ||
| continue; | ||
| } | ||
| let sym_idx = elf64_r_sym(reloc.r_info) as u32; |
There was a problem hiding this comment.
Isn't this already a u32?
| let sym_idx = elf64_r_sym(reloc.r_info) as u32; | |
| let sym_idx = elf64_r_sym(reloc.r_info); |
There was a problem hiding this comment.
Nope, Elf64_Xword is u64
There was a problem hiding this comment.
The elf64_r_sym returns u64 and sym_name takes u32 because symbol indices are 32-bit values in practice.
So the as u32 cast is always lossless so perhaps we could just have elf64_r_sym return u32 directly since the result is always 32 bits
| 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(); |
There was a problem hiding this comment.
Nit: avoid as conversion. If we're on x64, I think there should be a direct From instance?
| let addr = reloc.r_offset as usize + dyn_info.base_address(); | |
| let addr = usize::from(reloc.r_offset) + dyn_info.base_address(); |
There was a problem hiding this comment.
😢
error[E0277]: the trait bound `usize: From<u64>` is not satisfied
--> libdd-gotter/src/elf.rs:816:28
|
816 | let addr = usize::from(reloc.r_offset) + dyn_info.base_address();
| ^^^^^ the trait `From<u64>` is not implemented for `usize`
```
There was a problem hiding this comment.
Agreed. Its a 🦶 🔫 that unchecked casts are easier than checked.
| /// parses it with a valid sym_count calculated from DT_HASH nchain. | ||
| #[test] | ||
| #[cfg_attr(miri, ignore)] | ||
| fn test_sysv_hash_library_parsed_correctly() { |
There was a problem hiding this comment.
I wonder if it's reasonable to recompile a shared library each time just for a test. Would that be possible to pre-compile it instead as an asset and include it in the repo? Or maybe as an additional target of the crate and try to find it at test time using environment variable? Though maybe on a very small lib it's fast enough, but...
| #[cfg_attr(miri, ignore)] | ||
| fn test_dynamic_info_parses_loaded_library() { | ||
| let mut found = false; | ||
| iterate_libraries(|info, _| { |
There was a problem hiding this comment.
On all those tests that use iterate_libraries, do you rely on libc being loaded at least for them to not be flaky?
There was a problem hiding this comment.
Yes. iterate_libraries always returns at least the main executable (which has PT_DYNAMIC on dynamically linked binaries), so the iteration itself isn't flaky. But a couple of things here:
test_dynamic_info_parses_loaded_libraryneeds at least one object with PT_DYNAMIC + valid strtab/symtab/hash. The main executable could theoretically be statically linked with no dynamic section, makingfrom_phdrreturnNonefor everything.test_all_loaded_libraries_have_valid_sym_countis the same as abovtest_can_lookup_known_symbolneedsmalloc(or whatever symbol) to exist in a dynamically loaded library.
On our test runners, all of these seem to be okay, which seems to me like as long as our test runner images don't change, these should never be flaky.
But if you want these to be defensive, test_iterate_libraries_finds_loaded_objects could be the only hard assertion ( checks count > 0), and the others could skip gracefully if no library parses.
WDYT?
d224c5d to
a752f7f
Compare
aea0f42 to
66e5b36
Compare
a752f7f to
5e2abd9
Compare
…_symbol - DT_HASH (sysv) fallback: from_phdr no longer requires DT_GNU_HASH. Objects linked with --hash-style=sysv are now parsed by reading nchain from the sysv hash header. - elf64_r_type + is_got_pointer_reloc: only patch GLOB_DAT and JUMP_SLOT relocations. Skips non-pointer relocation types that would corrupt adjacent code/data if overwritten as 8-byte pointer slots. - hook_symbol: single-symbol convenience wrapper that composes dlsym, iterate_libraries, and patch_got_entries into one call.
5e2abd9 to
788f2de
Compare
66e5b36 to
31e53ca
Compare
| /// `info` must point to a valid `dl_phdr_info` from `dl_iterate_phdr`. | ||
| pub unsafe fn from_phdr(info: &dl_phdr_info) -> Option<Self> { | ||
| let phdrs = core::slice::from_raw_parts(info.dlpi_phdr, info.dlpi_phnum as usize); | ||
| let phdrs = slice::from_raw_parts(info.dlpi_phdr, info.dlpi_phnum as usize); |
There was a problem hiding this comment.
I know Rust doesn't require unsafe blocks inside unsafe functions, but I consider that a design flaw. Consider putting the actually unsafe parts inside blocks with safety comments to make it easier to audit
| // Can't estimate; allow any index and rely on strtab | ||
| // bounds checking in sym_name to catch bad accesses. | ||
| u32::MAX | ||
| (sym_count_fallback(symtab, strtab, sysv_hash), 0) |
| /// Narrow or PC-relative types (`R_X86_64_PC32`, `R_AARCH64_TLSDESC`, etc.) | ||
| /// are excluded since they have different widths and addend semantics | ||
| pub fn is_got_pointer_reloc(r_type: u32) -> bool { | ||
| // x86_64 |
There was a problem hiding this comment.
Should these be cfg guarded by platform?
| 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(); |
There was a problem hiding this comment.
Agreed. Its a 🦶 🔫 that unchecked casts are easier than checked.
3 participants
Stacked above refactor(got-patching): extract shared GOT-patching primitives into libdd-got-hook
Stacked under feat(crashtracking): retrieve c assert message for linux when __assert_fail is dynamically loaded
What does this PR do?
Adds three general improvements to
libdd-got-hookfor crash tracking (and beneficial to heap profiling):DT_HASH(SysV) fallback:DynamicInfo::from_phdrno longer requiresDT_GNU_HASH. Objects linked with--hash-style=sysvare now parsed by readingnchainfrom the SysV hash header. We jjust skipped these objects previously.elf64_r_type+is_got_pointer_relocnow only patchesGLOB_DATandJUMP_SLOTrelocations (pointer-sized GOT slots) on bothx86_64andaarch64. Non-pointer relocation types are now skipped, preventing potential corruption of adjacent code/data from writing 8 bytes into a 4-byte relocation field. Applied to bothlibdd-got-hook'spatch_got_entriesandlibdd-profiling-heap-gotter'sprocess_relocation. (this was a codex recommendation)hook_symbol: Single-symbol convenience wrapper that combinesdlsym+iterate_libraries+patch_got_entriesinto one call.Motivation
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.