feat(gotter)!: unpatch symbol functionality - #2342
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results✅ No issues found! 📦
|
71ed1ee to
dc757c4
Compare
BenchmarksComparisonBenchmark execution time: 2026-09-11 18:43:01 Comparing candidate commit b5deb14 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.
|
| cpu_model | git_commit_sha | git_commit_date | git_branch |
|---|---|---|---|
| Intel(R) Xeon(R) Platinum 8175M CPU @ 2.50GHz | b5deb14 | 1789151262 | gyuheon0h/unpatch-gotter |
| scenario | metric | min | mean ± sd | median ± mad | p75 | p95 | p99 | max | peak_to_median_ratio | skewness | kurtosis | cv | sem | runs | sample_size |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| receiver_entry_point/report/2644 | execution_time | 6.293ms | 6.432ms ± 0.064ms | 6.454ms ± 0.037ms | 6.479ms | 6.516ms | 6.533ms | 6.539ms | 1.32% | -0.543 | -0.957 | 1.00% | 0.005ms | 1 | 200 |
| scenario | metric | 95% CI mean | Shapiro-Wilk pvalue | Ljung-Box pvalue (lag=1) | Dip test pvalue |
|---|---|---|---|---|---|
| receiver_entry_point/report/2644 | execution_time | [6.423ms; 6.441ms] or [-0.138%; +0.138%] | None | None | None |
Baseline
Baseline benchmark details
Group 1
| cpu_model | git_commit_sha | git_commit_date | git_branch |
|---|---|---|---|
| Intel(R) Xeon(R) Platinum 8175M CPU @ 2.50GHz | 3f833f8 | 1789135942 | main |
| scenario | metric | min | mean ± sd | median ± mad | p75 | p95 | p99 | max | peak_to_median_ratio | skewness | kurtosis | cv | sem | runs | sample_size |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| receiver_entry_point/report/2644 | execution_time | 6.315ms | 6.374ms ± 0.023ms | 6.375ms ± 0.012ms | 6.385ms | 6.411ms | 6.439ms | 6.475ms | 1.57% | 0.528 | 2.283 | 0.36% | 0.002ms | 1 | 200 |
| scenario | metric | 95% CI mean | Shapiro-Wilk pvalue | Ljung-Box pvalue (lag=1) | Dip test pvalue |
|---|---|---|---|---|---|
| receiver_entry_point/report/2644 | execution_time | [6.371ms; 6.377ms] or [-0.051%; +0.051%] | None | None | None |
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: b5deb14 | Docs | View more details | Give us feedback! |
30f311c to
fe3f528
Compare
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
|
![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: fe3f528b44
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1bed325d8
ℹ️ 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".
91800af to
aaf32c6
Compare
aaf32c6 to
78478b3
Compare
gyuheon0h
changed the title
8a63381 to
c9368c1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a63381422
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4405231fd2
ℹ️ 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 slot = addr as *const AtomicUsize; | ||
| match unsafe { | ||
| (*slot).compare_exchange( |
There was a problem hiding this comment.
For supported absolute relocations, r_offset is not guaranteed to be usize-aligned; for example, a function-pointer field in a packed global produces an R_X86_64_64 relocation at an unaligned address, which is why the patch path uses read_unaligned/write_unaligned. Dereferencing that address as AtomicUsize requires atomic alignment, so unhooking such a slot invokes undefined behavior and can fault, especially on AArch64; use an alignment-safe conditional replacement strategy or exclude unaligned slots.
AGENTS.md reference: AGENTS.md:L70-L71
Useful? React with 👍 / 👎.
4405231 to
424b097
Compare
b789947 to
2603f92
Compare
| /// Every address in `hook_result.slots` must still point into a | ||
| /// currently-mapped region. No library whose GOT was patched may be | ||
| /// `dlclose`d before this function returns. |
There was a problem hiding this comment.
this is a hard to satisfy requirement. Need to protect against when this happens in the API explicitly
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2603f92e89
ℹ️ 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".
| // JUMP_SLOT/GLOB_DAT are always pointer-aligned, but | ||
| // R_X86_64_64/R_AARCH64_ABS64 entries in data sections may not be. | ||
| // SAFETY: `addr` points to a writable GOT slot in mapped memory. | ||
| unsafe { core::ptr::write_unaligned(addr as *mut usize, original_value) }; |
There was a problem hiding this comment.
When a supported R_X86_64_64 or R_AARCH64_ABS64 function-pointer slot is unaligned, a concurrent caller can observe a torn value during this write_unaligned and jump to an invalid address, despite the new contract explicitly declaring concurrent calls safe. The fresh evidence after the prior atomic-alignment finding is that the final implementation switched to a non-atomic unaligned write while retaining that guarantee; either require callers to be quiesced or exclude unaligned slots/use a synchronization strategy that actually makes replacement atomic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yeah... I guess there isnt really anything we can do here since we can't lock function calls through the GOT... and we physically cannot have atomic guarantees on non-aligned ptrs
There was a problem hiding this comment.
Doesn't that contradict the documentation of this function (writes are aligned and pointer-sized thus atomic)? Or at least it's an addendum that could be added? Just to be sure to understand the situation: if a thread goes through the GOT slot, then all good (aligned, atomic, sees either the old or the new function). However for direct relocations, this is not the case. So a concurrent thread reading the target of a relocation could observe an inconsistent state, right?
2603f92 to
a7e16b3
Compare
a7e16b3 to
b5deb14
Compare
There was a problem hiding this comment.
What is the scenario for unloading the crashtracker lib? Can it happen in the middle of a normal run, or is it something like at exit (if we're unloaded in the wrong order, this causes spurious crashes)?
The issue with unhooking is that the lifecycle for pure relocations that don't go through the GOT/PLT seems to be quite delicate:
Overwriting with the wrong value
For example, let's say you have static like
static void *(*allocate)(size_t) = malloc;
Then, during the lifetime of the program, there's a new assignment allocate = my_personal_allocator. During unhooking, we might accidentally restore malloc because we look at the original relocations, but those that aren't modified if a static variable gets updated. So it's not because we've initially patched a relocation target that it still contains the same thing. I guess we can just check the value of the target first and compare it to hook_symbol, although it introduces new fun data races.
Data races
Since we might overwrite mutable memory that is concurrently used, with misaligned pointer writes, there's no protection at all against data races. This is UB and could very well happen if another thread is trying to read the variable. Maybe we could just skip patching unaligned locations? I really don't expect this to happen a lot, especially for something like assert.
Stale data
Another possibility is that the value has been copied somewhere else. For example in a Vec<callbacks> from the static, or even just in a register. Those values can't be traced at all, and thus can't be "unhooked". They will continue to point back to the original hook, which might not exist anymore.
Should we patch relocations?
I don't know all the tradeoffs involved here, but I feel if we can get our way in practice without touching relocations at all, our life will be easier/safer. It means we'll miss some calls, and I guess we would have to see if it matters or not in practice (like if 0.01% of calls to assert are done through direct relocations, then 🤷♂️ ).
Not patching unaligned relocation target would already get rid of the data race issue (at least most of it, because there's still a TOCTOU when some threads might have already loaded the old hook address locally in a register as we unhook, but those are 1. unavoidable I guess and 2. the window for those is much, much smaller - we would have to unhook and unload the module between a thread loading the address and actually jumping to the function).
| // If /proc/self/maps isn't available (or the page isn't in it, | ||
| // which shouldn't happen for a mapped GOT page) fall back to | ||
| // PROT_READ, the RELRO'd default. Tighter than leaving pages RW. | ||
| let orig = self.original_prot(aligned).unwrap_or(PROT_READ); |
There was a problem hiding this comment.
This means that if something else (imagine a libdatadog-like library, or the host program itself) is relying on this mapping being writeable, this might create strange memory protection errors if we restore a stronger permission than what was here originally. Now I assume this will be an exceptional situation (writeable required + no /proc/self/maps), and I still think restoring READ only is the right thing to do security-wise, but I wonder if that will come to bite us some day with a strange bug. Maybe we could document this behavior, e.g. in the README or somewhere else?
| /// If [`slots_failed`] is non-zero, one or more GOT entries still point at | ||
| /// the hook function because `mprotect` was rejected (e.g. a seccomp policy | ||
| /// installed after hooking). Those slots remain live pointers into the | ||
| /// library being unloaded; the caller **must not** unload until all slots |
There was a problem hiding this comment.
It's a bit strange to talk about "the caller" for a struct documentation. Would the actual function be a better place to put this documentation? Should this be a safety condition?
| /// | ||
| /// `hook_fn` must point to a function with the same calling convention | ||
| /// and signature as the symbol being hooked. The patching is permanent. | ||
| /// and signature as the symbol being hooked. |
There was a problem hiding this comment.
Nit: mention in this function's documentation the unhook function?
| /// hook's own library regardless of PIE vs non-PIE (where `dlpi_addr` | ||
| /// may be 0 for the main executable). | ||
| /// | ||
| /// We MUST NOT dlopen a library that contains the hook symbol during the duration |
There was a problem hiding this comment.
Nit: should that be part of a proper # Safety section? Or is it not really unsafe but just leads to unwanted behavior
| // Reserve before patching so push is guaranteed not to allocate | ||
| // after any GOT entry in this library has been overwritten. | ||
| let count = count_matching_got_entries(&dyn_info, symbol_name_bytes); | ||
| unsafe { (*slots_ptr).reserve(count) }; |
There was a problem hiding this comment.
Does this have to be unsafe code? Could we share a mutable reference, use reserve safely, and only convert it into a pointer just in time when calling patch_got_entries? (maybe not, but asking just in case)
There was a problem hiding this comment.
In fact, patch_got_entries takes an &mut, so I suspect slots can stay a safe reference all along?
| for reloc in relocs { | ||
| try_count(reloc.r_info); | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit (style): instead of having to make a closure here, I think you could just chain iterators instead, something like for reloc in dyn_info.rels().into_iter().chain(dyn_info.relas()).chain(dyn_info.jmprels()) { ...count... }.
| // JUMP_SLOT/GLOB_DAT are always pointer-aligned, but | ||
| // R_X86_64_64/R_AARCH64_ABS64 entries in data sections may not be. | ||
| // SAFETY: `addr` points to a writable GOT slot in mapped memory. | ||
| unsafe { core::ptr::write_unaligned(addr as *mut usize, original_value) }; |
There was a problem hiding this comment.
Doesn't that contradict the documentation of this function (writes are aligned and pointer-sized thus atomic)? Or at least it's an addendum that could be added? Just to be sure to understand the situation: if a thread goes through the GOT slot, then all good (aligned, atomic, sees either the old or the new function). However for direct relocations, this is not the case. So a concurrent thread reading the target of a relocation could observe an inconsistent state, right?
|
@yannham Yeah. I spent (too much -- IMO shouldve bailed earlier) time trying to force an "unhook" API, but making unhooks that work all the time for all cases and is always consistent and reliable after a "permanent" GOT modification has been a struggle. The reason I started this is because PHP apparently sometimes unloads the tracer, which the customer application dynamically links to. So, if we ever patch something that gets called later on, it is a segfault as our patch function is no longer mapped. I think I will change the direction of this to implement an unhook that is only to be called if crashtracker is unloaded -- making it clear in the docs that its not intended to be called regularly, and GOT patching through libdd-gotter shouldbe considered permanent. |
2 participants
What does this PR do?
This pr adds
unhook_symbolAPI tolibdd-gotterthat reverses a previoushook_symbolcall by restoring GOT entries that currently point at the hook function back to the original address. This only restores entries whose current value matcheshook_fn, making it safe to call even if libraries were loaded/unloaded after the original hook or entries were independently modified.Motivation
Embedders that load libdatadog as a shared library and later
dlcloseit would leave other modules' GOT slots pointing athook_assert_failinside the now-unmapped image. A subsequent Cassert()in any patched module would jump to stale code.Additional Notes
Anything else we should know when reviewing?
How to test the change?
Unit tests