feat(crashtracking): retrieve c assert message for linux when __assert_fail is dynamically loaded - #2268
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. |
gyuheon0h
changed the title
f3e6979 to
6e7f47d
Compare
BenchmarksComparisonBenchmark execution time: 2026-07-30 21:47:57 Comparing candidate commit 34cd4ce in PR branch Found 5 performance improvements and 5 performance regressions! Performance is the same for 167 metrics, 10 unstable metrics.
|
|
13ad896 to
28b571f
Compare
gyuheon0h
changed the title
__assert_fail is dynamically loaded
gyuheon0h
changed the title
__assert_fail is dynamically loaded__assert_fail is dynamically loaded
6b2f473 to
7568fed
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: 7568fed720
ℹ️ 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".
217836f to
b3f0cf7
Compare
|
Have yet to look at it, but as a preamble, do you think there's some way to reuse or share the code from libdd-profiling-heap-gotter? It seems some type definitions at least, and probably some functions as well are very similar, if not the same. |
@yannham Yeah. Maybe I shouldn't have opened this yet. |
b8a35c0 to
0af85cb
Compare
0af85cb to
4b6482a
Compare
c55499a to
2dd85ed
Compare
822405c to
6ab7768
Compare
4618f83 to
8713e2e
Compare
0624b47 to
a72626c
Compare
8713e2e to
0f700c2
Compare
a72626c to
fe9884f
Compare
0f700c2 to
934d93c
Compare
b5959b0 to
48971da
Compare
5e08f80 to
e2110f9
Compare
48971da to
4156403
Compare
e2110f9 to
cad573a
Compare
d24e571 to
6dbad5d
Compare
cad573a to
6264ccb
Compare
6dbad5d to
a26fde9
Compare
6264ccb to
d224c5d
Compare
0054745 to
34cd4ce
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34cd4cebfb
ℹ️ 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 message_ptr = if message_ptr.is_null() { | ||
| super::assert_interceptor::take_assert_message_ptr() | ||
| } else { | ||
| message_ptr |
There was a problem hiding this comment.
If the process has a Rust panic that is caught after crashtracker initialization, the panic hook leaves PANIC_MESSAGE non-null until the next signal; when a later C assert() aborts, hook_assert_fail has already stored the assertion details, but this branch keeps the stale panic text and never reads the assert message, so the crash report describes the old panic instead of the actual assertion failure. Prefer the assert message for SIGABRT/assert-hook crashes or clear stale panic state before ignoring the new message.
AGENTS.md reference: AGENTS.md:L70-L73
Useful? React with 👍 / 👎.
| let message = format_assert_message(assertion_str, file_str, line, function_str); | ||
| store_assert_message(message); |
There was a problem hiding this comment.
When the intercepted assertion fires from allocator-adjacent code, such as a libc/malloc assertion after heap corruption or while the asserting thread holds an allocator lock, these calls allocate a Rust String/Box before tail-calling the real __assert_fail. That can re-enter or block on the allocator before libc gets to abort, so the process can hang and emit no crash report; capture raw pointers or a fixed-size buffer here and defer formatting until the forked collector runs.
AGENTS.md reference: AGENTS.md:L70-L75
Useful? React with 👍 / 👎.
| libdd_got_hook::hook_symbol( | ||
| c"__assert_fail", | ||
| hook_assert_fail as *const () as usize, | ||
| &mut orig_addr, |
There was a problem hiding this comment.
In embedders that load libdatadog as the supported .so/FFI library and later dlclose it during extension unload or reload, this permanent GOT patch leaves other modules' __assert_fail slots pointing at hook_assert_fail inside the unloaded image. A subsequent C assert() in any patched module will jump to unmapped/stale code instead of libc, so the hook needs an unpatch/disable-on-unload strategy or the library must be made non-unloadable before patching external GOTs.
AGENTS.md reference: AGENTS.md:L70-L74
Useful? React with 👍 / 👎.
d224c5d to
a752f7f
Compare
9829ca0 to
86a6558
Compare
a752f7f to
5e2abd9
Compare
Hook __assert_fail via GOT patching (using libdd-got-hook) during crashtracker init. When a C assert() fails, the hook captures the assertion expression, file, line, and function name before the process aborts via SIGABRT. The signal handler includes this message in the crash report. Only supported on 64-bit Linux; no-op on other platforms.
5e2abd9 to
788f2de
Compare
2 participants
Stacked above: feat(got-hook): add DT_HASH fallback, relocation type guard, and hook_symbol
What does this PR do?
When a process crashes due to a C assert() failure, the crash report now includes the assertion expression string (
Assertion failed: (x > 0), function foo, file bar.c, line 42) in error.message, instead of just reporting a bare SIGABRT.When a shared library calls an external function like
__assert_fail, it doesn't jump directly to the target. Instead, it jumps through the global offset table. Every shared library has its own GOT with its own slot for each external function it calls.We overwrites the slots at runtime for
__assert_failso calls are redirected to our hook function. My initial thought was to useLD_PRELOADbut this approach works no matter the load order.I was inspired by
libdd-profiling-heap-gotterwhich does something similar to interceptmalloc/freefor heap profiling.During
crashtracker::init(),install_assert_hook()PT_DYNAMICsegment to locate its relocation tablesWhen a C
assert()fails:The hook captures the assertion expression, file name, line no, func name, and stores it through an atomic pointer:
__assert_fail()implementation.abort(), which raises SIGABRT.Unfortunately, this doesn't work everywhere
__assert_failhas no GOT entry to patch. The hook installs silently as a no-op.install_assert_hook()compiles to an empty function.Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
How to test the change?