fix(elf): apply ELF relocations to debug sections in relocatable objects - #1060
fix(elf): apply ELF relocations to debug sections in relocatable objects#1060darktorres wants to merge 4 commits into
Conversation
b34cdba to
a951edc
Compare
|
Good catch — fixed in the latest push. |
Same fix as getsentry#1060, backported onto the 13.8.0 tag so it stays semver-compatible with sentry-cli 3.7.0's `symbolic = "13.1.1"` requirement (Cargo silently ignores a [patch] whose version falls outside the dependency's declared range, so patching against current master here would produce an unpatched build). This branch exists only to build a locally-patched sentry-cli for wiRedPanda's own CI until the upstream fix ships in a release; see getsentry#1060 for the real PR and full root-cause writeup.
|
Re the |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a951edc. Configure here.
Same fix as getsentry#1060, backported onto the 13.8.0 tag so it stays semver-compatible with sentry-cli 3.7.0's `symbolic = "13.1.1"` requirement (Cargo silently ignores a [patch] whose version falls outside the dependency's declared range, so patching against current master here would produce an unpatched build). This branch exists only to build a locally-patched sentry-cli for wiRedPanda's own CI until the upstream fix ships in a release; see getsentry#1060 for the real PR and full root-cause writeup.
Unlinked relocatable objects (ET_REL, i.e. plain .o files) leave every
DW_FORM_line_strp/DW_FORM_strp offset field in .debug_line/.debug_info as a
zero placeholder, with a companion R_*_32/R_*_64 relocation recording the
real offset into .debug_line_str/.debug_str (those sections' final layout
isn't settled until link time, since they can be merged/deduplicated across
translation units). ElfObject::parse already parses these relocations into
elf.shdr_relocs, but nothing ever applied them before handing section bytes
to gimli, so every such field silently read back as offset 0.
This is usually invisible for DW_AT_comp_dir, since GCC/Clang tend to place
comp_dir's string first (so its unrelocated placeholder and its real
relocated offset both happen to be 0), but it corrupts every other
directory/file-table entry in a DWARF5 line program's include_directories
and file_names tables, and any DW_FORM_strp-encoded attribute in .debug_info
more generally. When the (wrongly resolved) path happens to be a directory
that exists on disk, `debug-files bundle-sources` / `debug-files upload
--include-sources` hard-errors trying to open it as a file ("Is a
directory") instead of resolving -- or skipping -- the source correctly.
Confirmed with GCC 13/15 and Clang, DWARF32/64, x86_64/aarch64: reading an
affected .o directly with readelf/llvm-dwarfdump/gdb all correctly resolve
the same bytes (they apply the relocations), and stripping .rela.debug_line
from an otherwise-correct object reproduces the identical corruption in
readelf's own dump -- confirming relocation application is exactly the
missing step. Already-linked objects (ET_EXE/ET_DYN, including objcopy
--only-keep-debug output) are unaffected, since the linker resolves these
relocations itself.
Fix: after loading (and decompressing) a DWARF section's bytes for a
relocatable object, look up any relocations targeting that section from the
already-parsed elf.shdr_relocs and patch them into an owned copy before
handing the bytes to gimli. Scoped to the R_*_32/R_*_64 "absolute value"
relocation types actually used for debug-string offsets on ELF's two most
common architectures; other relocation types are left as before.
a951edc to
e76dd89
Compare
|
Good catch too — also fixed in the latest push. |
… fix Restores --include-sources for both Linux debug-file uploads, now backed by a from-source sentry-cli 3.7.0 build patched with the actual root-cause fix (symbolic-debuginfo's ELF loader was parsing but never applying ELF relocations to debug sections in unlinked .o files, so every DW_FORM_line_strp/strp offset silently read back as the section's own zero placeholder). Fix submitted upstream at getsentry/symbolic#1060; the CI patch branch tracks the same fix on a 13.8.0-based branch (semver-compatible with sentry-cli 3.7.0's `symbolic = "13.1.1"` requirement -- patching against current master silently no-ops, since Cargo ignores a [patch] whose version falls outside the dependency's declared range). Verified end-to-end: the released recipe (fresh clone, patched Cargo.toml, `cargo build --release`) builds a working sentry-cli binary in ~5-8 minutes and correctly bundles sources for every previously- crashing object (real production .o files, and x86_64/aarch64 synthetic repros), with zero regression on already-working linked binaries. Windows and macOS are untouched -- PDB and Mach-O/dSYM aren't affected by this ELF-specific bug, so they keep downloading the official release. Once getsentry/symbolic#1060 ships in a release, this can revert back to downloading the official sentry-cli binary on Linux too.
…, alloc) - Add a regression test with a purpose-built unlinked .o fixture. Verified it actually catches the bug: fails against the pre-fix code (asserting precise dir_str()/name_str() equality rather than a substring/suffix match, since the corrupted output for this fixture happens to still satisfy a weaker check), passes with the fix. - Document why r_addend is used directly (assumes a section-relative symbol, i.e. value 0 -- true for every compiler's debug-section relocations, not guaranteed by the ELF format itself) and why a REL (non-RELA) relocation section degrades safely to a no-op rather than being handled (both x86_64 and AArch64 are RELA-only per their psABIs, so this never currently triggers, but would silently stay a no-op rather than corrupt data if extended to an architecture that isn't). - Avoid a small heap allocation per relocation: write into a fixed [u8; 8] stack buffer and slice it, instead of `.to_vec()`.
…, alloc) Same fix as the upstream PR (getsentry#1060), backported here to stay in sync: a regression test (verified to fail pre-fix, pass post-fix), documented r_addend/RELA assumptions, and a fixed-buffer instead of a per-relocation heap allocation.
|
Pushed a follow-up addressing gaps from my own review after the two bot-caught fixes:
|
Doc comments/tests had drifted into a more verbose style than the rest of the crate uses; trimmed to match, fixed an inaccurate claim that relocation sections are matched by name (they're matched via sh_info), and added the missing CHANGELOG entry every other landed fix carries.
… fix 5.2.2's Linux deploy jobs failed at `sentry-cli debug-files upload --include-sources` with "failed to write source bundle: Is a directory (os error 21)", aborting before Publish Ubuntu -- the release has no Linux AppImages attached. Root cause: symbolic-debuginfo's ELF loader (a sentry-cli dependency) parses ELF relocations for debug sections into elf.shdr_relocs but never applies them before handing section bytes to gimli. Every DW_FORM_line_strp/strp offset in an unlinked relocatable object (.o, ET_REL) is a zero placeholder resolved by a relocation at link time, so it silently reads back as offset 0, corrupting directory/file resolution for any GCC/Clang-compiled .o -- not specific to Qt or wiRedPanda. Fix submitted upstream: getsentry/symbolic#1060. Until that ships in a release, build sentry-cli 3.7.0 from source in deploy.yml (Linux only) with the same fix patched in from a pinned branch, and restore --include-sources. Windows and macOS are untouched (PDB and Mach-O/dSYM aren't affected by this ELF-specific bug).
… fix 5.2.2's Linux deploy jobs failed at `sentry-cli debug-files upload --include-sources` with "failed to write source bundle: Is a directory (os error 21)", aborting before Publish Ubuntu -- the release has no Linux AppImages attached. Root cause: symbolic-debuginfo's ELF loader (a sentry-cli dependency) parses ELF relocations for debug sections into elf.shdr_relocs but never applies them before handing section bytes to gimli. Every DW_FORM_line_strp/strp offset in an unlinked relocatable object (.o, ET_REL) is a zero placeholder resolved by a relocation at link time, so it silently reads back as offset 0, corrupting directory/file resolution for any GCC/Clang-compiled .o -- not specific to Qt or wiRedPanda. Fix submitted upstream: getsentry/symbolic#1060. Until that ships in a release, build sentry-cli 3.7.0 from source in deploy.yml (Linux only) with the same fix patched in from a pinned branch, instead of downloading the official unpatched binary. --include-sources itself is untouched -- it was already present in deploy.yml and stays that way; only the sentry-cli binary running it changes. Windows and macOS are untouched (PDB and Mach-O/dSYM aren't affected by this ELF-specific bug).
… fix 5.2.2's Linux deploy jobs failed at `sentry-cli debug-files upload --include-sources` with "failed to write source bundle: Is a directory (os error 21)", aborting before Publish Ubuntu -- the release has no Linux AppImages attached. Root cause: symbolic-debuginfo's ELF loader (a sentry-cli dependency) parses ELF relocations for debug sections into elf.shdr_relocs but never applies them before handing section bytes to gimli. Every DW_FORM_line_strp/strp offset in an unlinked relocatable object (.o, ET_REL) is a zero placeholder resolved by a relocation at link time, so it silently reads back as offset 0, corrupting directory/file resolution for any GCC/Clang-compiled .o -- not specific to Qt or wiRedPanda. Fix submitted upstream: getsentry/symbolic#1060. Until that ships in a release, build sentry-cli 3.7.0 from source in deploy.yml (Linux only) with the same fix patched in from a pinned branch, instead of downloading the official unpatched binary. --include-sources itself is untouched -- it was already present in deploy.yml and stays that way; only the sentry-cli binary running it changes. Windows and macOS are untouched (PDB and Mach-O/dSYM aren't affected by this ELF-specific bug).
loewenheim
left a comment
There was a problem hiding this comment.
Hi, thank you very much for the contribution!
|
Out of interest, how did you hit upon this problem? Are you trying to upload relocatable ELF files to Sentry? |
|
Hi, I got hit by this issue in this run https://github.com/GIBIS-UNIFESP/wiRedPanda/actions/runs/33291256790/job/99203154905 |
Per review feedback: the removed sentence was PR context, not documentation of the function's behavior.

Summary
Unlinked relocatable objects (
ET_REL, i.e. plain.ofiles) leave everyDW_FORM_line_strp/DW_FORM_strpoffset field in.debug_line/.debug_infoas a zero placeholder, with a companionR_*_32/R_*_64relocation recording the real offset into.debug_line_str/.debug_str(those sections' final layout isn't settled until link time, since they can be merged/deduplicated across translation units).ElfObject::parsealready parses these relocations intoelf.shdr_relocs, but nothing ever applied them before handing section bytes togimli, so every such field silently read back as offset 0.This is usually invisible for
DW_AT_comp_dir, since GCC/Clang tend to placecomp_dir's string first in these sections (so its unrelocated placeholder and its real relocated offset both happen to be 0), but it corrupts every other directory/file-table entry in a DWARF5 line program'sinclude_directories/file_namestables, and anyDW_FORM_strp-encoded attribute in.debug_infomore generally. When the wrongly-resolved path happens to coincide with a directory that exists on disk,sentry-cli debug-files bundle-sources/debug-files upload --include-sourceshard-errors trying to open it as a file ("Is a directory (os error 21)") instead of resolving -- or gracefully skipping -- the source. When it doesn't, the source is silently attributed to the wrong file, or a source lookup silently fails ("no files found").Root cause, verified independently
gimli's DWARF5 line-program parser directly: cursor advancement/byte-consumption for every field is exactly correct; the value read for everyline_strp/strpfield is genuinely0in the section bytes handed to it -- verified againstreadelf -r's dump of the corresponding.rela.debug_line/.rela.debug_infosection, which shows the real (non-zero) relocation targets untouched at those exact file offsets.readelf,llvm-dwarfdump, andgdball correctly resolve the exact same bytes (they apply the relocations themselves, as expected for any relocation-aware DWARF consumer). Stripping.rela.debug_linefrom an otherwise-correct object (objcopy --remove-section=.rela.debug_line) reproduces the identical corruption inreadelf's own dump of that same file -- i.e. removing relocation application from a working tool breaks it the same way, confirming this is exactly the missing step rather than a coincidence.-fno-merge-debug-strings, with plain/split-DWARF (-gsplit-dwarf) and compressed (-gz) debug sections -- same mechanism every time.ET_EXEC/ET_DYN, including PIE executables, shared libraries, andobjcopy --only-keep-debugoutput) are unaffected, since the linker resolves these relocations itself before symbolic ever sees the bytes.Fix
After loading (and decompressing) a DWARF section's bytes for a relocatable object, look up any relocations targeting that section from the already-parsed
elf.shdr_relocsand patch them into an owned copy before handing the bytes togimli. Scoped to theR_*_32/R_*_64"absolute value" relocation types actually used for debug-string offsets on ELF's two most common architectures (x86_64, aarch64); other relocation types are left untouched, matching prior behavior.Testing
cargo test -p symbolic-debuginfo --features elf: all suites pass (120 + 49 + 4 + 2 + 10 unit/doc tests, 0 failed).cargo clippy -p symbolic-debuginfo --features elf --no-deps: clean.cargo fmt -p symbolic-debuginfo -- --check: clean.sentry-clilinked to this patchedsymbolic-debuginfo:debug-files bundle-sourcesnow produces a correct source bundle (verified manifest + extracted file contents match the real source) for objects that previously hard-errored, with zero regression on already-working linked binaries/shared libraries.