symtab/elf: bound symbols by st_size to avoid misattributing stripped code#4
Open
gecube wants to merge 1 commit into
Open
symtab/elf: bound symbols by st_size to avoid misattributing stripped code#4gecube wants to merge 1 commit into
gecube wants to merge 1 commit into
Conversation
… code SymbolTable.Resolve mapped a PC to the symbol with the greatest value <= pc with no upper bound. In a stripped binary that keeps only a handful of exported .dynsym symbols (e.g. an Envoy sidecar that exports symbols for dlopen'd Go/Lua modules while .symtab is stripped), the nearest surviving symbol can sit arbitrarily far below the PC. As a result, samples that actually land in stripped-out functions were attributed to an unrelated exported symbol, and removing that symbol just shifted the samples to the next surviving one. st_size was already parsed out of each Elf symbol but discarded. Keep it and bound each symbol to [Value, Value+Size): when the size is known and the PC falls past the symbol's end, resolve to "" (unknown) instead of the nearest symbol. Symbols with size 0 (unknown) keep the previous nearest-symbol behavior, so nothing regresses for binaries that omit sizes. Also populate Value/Size in the 32-bit symbol path, which previously left the symbol value unset. Signed-off-by: Gaál György <gb12335@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Reported downstream in coroot/coroot-node-agent#262: when profiling a stripped binary (an Envoy/Istio sidecar) where most symbols are removed but a few dynamic symbols are kept in
.dynsym(needed by dlopen'd Go/Lua modules), the profiler shows large amounts of time in exported functions that are never actually called — e.g.envoyGoFilterLogLevel. Compiling that function out just moves the time to the next surviving exported symbol (luaopen_jit).Root cause
SymbolTable.ResolveusesPCIndex.FindIndex(addr), which returns the symbol with the greatestValue <= addrand no upper bound. The only guard isaddr < firstSymbolValue. In a stripped binary whose symbol table holds only a handful of exported.dynsymentries, the nearest surviving symbol below a PC can be arbitrarily far away, so every sample that lands in a stripped-out function is attributed to whichever exported symbol precedes it.Each ELF symbol's
st_sizewas already read ingetSymbols64/getSymbols32but explicitly discarded (//Size: ... // not used), so there was no information to bound a symbol's range.Fix
st_sizeonSymbolIndexand carry it intoFlatSymbolIndex.Sizes(parallel toNames/Values).Resolve, when the matched symbol's size is known, reject PCs at or beyondValue+Sizeand return""(unknown) instead of the nearest symbol. This is what the downstream issue author asked for: show unknown rather than an unrelated symbol.st_size == 0(size unknown, e.g. some hand-written asm) keep the previous nearest-symbol behavior — no regression for binaries that omit sizes.symbols[i].Value; populateValue/Sizethere too.GoTable.Resolvealready applies an equivalent upper bound viaIndex.End; this brings the plain ELFSymbolTablein line.Tests
TestSymbolTableResolveBoundsencodes the issue scenario (two exported symbols with real sizes plus a size-0 symbol) and asserts PCs in the gaps resolve to"", PCs inside a symbol resolve to its name, and size-0 symbols retain legacy behavior.TestElfSymbolComparisonacross alltestdata/elfsfixtures still passes (Names/Values are unchanged;Sizesis additive).Reproduction mechanics
A stripped ELF that mirrors the Envoy case (default-visibility functions exported via
-rdynamic, everything else hidden, thenstrip --strip-allremoving.symtabbut keeping.dynsym), resolved through this package:Before:
After: the three
hidden_*PCs resolve to<unknown>instead of an unrelated exported symbol.