Repository navigation
Fix/review 2026 08 - #156
Merged
Merged
Conversation
Five-phase execution plan from a whole-repo design/perf/cleanliness review: Phase 1 (C1-C4) correctness bugs Phase 2 (D1) stop mutating system shared libraries in place Phase 3 (E1-E5) tracer and CLI cleanups Phase 4 (P1-P4) report and enumeration performance Phase 5 (S1-S3) small cuts Highest value: C1 (install can leave a system binary deleted if the shim copy fails after the original has been relocated) and D1 (mergeLibraryDebugInfo eu-unstrips live system libraries, so rpm -V reports them modified and a crash between backup and restore leaves the host permanently changed). D1's replacement — resolving uprobe file offsets ourselves and attaching via UprobeMultiOptions.Addresses — was validated read-only on the VM before planning: build-ids match, the debug symtab is a strict superset (1040 vs 911 FUNC names on libgmp, 129 internal mpn_* gained), and the computed offset disassembles to an instruction boundary. It also turned up the trap that makes the whole phase dangerous to get wrong: the two files agree on PT_LOAD p_vaddr but not p_offset, and every image on the test host has p_offset == p_vaddr, so e2e cannot catch a botched conversion. Hence the mandatory build-id staleness guard and a unit test cross-checking against cilium's own by-name resolution. Includes the Phase 0 baseline, the VM e2e procedure, and a Rejected section recording three review findings that collide with decisions already taken in fix/audit-2026-08 (A4, A10, B4), so they don't get re-derived.
relocateOriginal moves the target out of its own path; the shim only lands there later, in finishInstall. Between those two points install could return an error with the path simply empty — the binary gone from the system. ENOSPC, a read-only /usr, or a truncated shim all reach that window, and installMany just logs the failure and moves on to the next target, so nothing notices. enumerateAndPersist had an ad-hoc `_ = move(safePath, realTarget)` covering only its own enumeration failure, with a comment noting that a later WriteFuncList failure deliberately did NOT roll back. Replace it with a single defer in install, armed as soon as relocateOriginal succeeds and disarmed once finishInstall returns, so the whole window is covered symmetrically. rollbackInstall does more than the old one-liner: it restores any system libraries already unstripped in place, clears the sidecars keyed on safePath, and removes the multicall alias (installMulticallSymlink now returns the link it created for exactly this). A rollback that itself fails is reported as CRITICAL on stderr instead of being folded into the returned error, which installMany would otherwise print as one more line in a list of failed targets. relocateOriginal keeps its own internal rollback: it runs before install has anything to disarm. Test injection point is worth recording: findShimBinary only os.Stats its candidates, so pointing FUNKOVERAGE_SHIM at a directory is accepted and then fails inside copyFile's io.Copy with EISDIR — precisely the target window, and it behaves identically whether or not the test runs as root, unlike an unreadable file.
uninstall cleared .funcs.json and (via restoreLibraryBackups) .libbackup.json, but left .filter.json and .shimbin.json behind in SAFE_BIN_DIR, contradicting AGENTS.md's "removes sidecars". Factor the three safePath-keyed sidecars into clearSidecars, shared with C1's rollbackInstall. The library-backup sidecar deliberately stays out of it: restoreLibraryBackups owns that one, and must put the libraries back before the record of where their backups live is thrown away. Caught by the assertSafeBinDirEmpty helper added for C1's rollback test — .shimbin.json was the survivor in TestInstallUninstall. .filter.json only leaks when a filter was actually set, which no existing test did.
enumerateOne returned the first non-empty symbol list, checking the runtime file before its external debug file. For a stripped/debuglinked pair that is backwards: distro packaging moves the full .symtab into the debug file and leaves the runtime file with only .dynsym, so every LOCAL/static function lives in exactly one of the two. Any .dynsym export at all was enough to stop the search and hide the rest — a CPython extension module resolved to just its PyInit_* entry point. Reorder to debug symtab -> own symtab -> DWARF. The superset premise was checked on the VM across seven images first, counting defined functions only (FUNC, Ndx != UND, non-zero size — what SymtabFunctions keeps): image runtime debug runtime-only libgmp.so.10.5.0 874 997 0 libcrypto.so.3 5859 13101 0 libssl.so.3 603 2512 0 gzip 99 99 0 bzip2 39 39 0 squid 15174 15174 0 _bz2.cpython-313 1 19 0 Zero runtime-only symbols anywhere, so the reorder cannot lose anything. _bz2 reproduces plan.md §3 exactly (1 vs 19); libssl is the biggest win. Images with no debug file are untouched — externalDebugPath returns "". The test reuses buildStrippedLibFixture, which already builds this shape for the merge tests, and fails without the reorder with got [lib_public_func]. externalDebugPath is now called unconditionally, one extra elf.Open per image: BenchmarkEnumerateFunctions 6 523 991 -> 6 688 722 ns/op (+2.5%). P4 (parallel library enumeration) repays it. plan.md §3 is pruned to the one part this does not close: the openQA coverage_setup.pm debuginfo gap, which lives in os-autoinst-distri-opensuse.
ldd invokes the target binary's own PT_INTERP with
LD_TRACE_LOADED_OBJECTS=1, so `funkoverage install` — which runs as root —
executes code out of the very binary it is instrumenting. It also costs a
fork+exec per target.
Replace it with a DT_NEEDED walk in cmd/libdeps.go that applies ld.so's own
resolution rules: DT_RPATH/DT_RUNPATH with $ORIGIN and $LIB expansion,
LD_LIBRARY_PATH, a direct parse of the new-format /etc/ld.so.cache, and the
default library directories. ParseLddLibraries moves into the same file and
stays as the fallback for any SONAME the walk cannot resolve, so a host whose
layout is not fully understood keeps today's exact behaviour instead of
quietly enumerating a subset.
Two ld.so rules matter more than they look:
- glibc-hwcaps subdirectories are searched before the directory itself,
and openSUSE ships x86-64-v3 builds of libz, libzstd, libjpeg, liblz4
and brotli. Resolving to the baseline file while the process maps the v3
one attaches the uprobe to the wrong inode, which never fires and
reports 0% for that library with no error anywhere.
- $ORIGIN is relative to the realpath of the file ld.so opened, not to the
path as given.
Both were found by the new test, which diffs the resolver against ldd over
the host's own binaries; it matches on all 319 of /usr/bin here, with no
fallbacks. BenchmarkEnumerateFunctions 6.52ms -> 4.25ms.
UprobeMulti resolves the whole name batch up front and fails on the first miss, so a single unresolvable name silently costs an entire library's coverage. C3 made enumeration prefer the external debug file's symtab, which exposed this on distributions where the install-time eu-unstrip merge is refused (Leap 16: "cannot find matching section for [10] '.rela.dyn'"). There the debug-only names are not in the file the kernel maps, and openssl fell from 1824 to 181 called functions. attachableSymbols pre-filters each image's batch against the names the mapped file can actually resolve, keeping every survivor paired with the cookie flattenFuncs assigned it. ResolvableFuncNames deliberately mirrors cilium's own predicate, since that is the cache UprobeMulti consults. Leap 16 openssl is back to 1824 called against C3's richer 16273 denominator; Tumbleweed, where the merge succeeds and nothing is filtered, is unchanged at 3969/16297. Both container suites pass 9/9.
…(D1)
funkoverage ran eu-unstrip over /lib64/libfoo.so.N on the live host so that
uprobe attach could resolve a library's internal functions by name. That made
rpm -V report every touched library as modified, left the host permanently
modified if the process died between backup and restore, and raced between
concurrent installs.
Instead, install time now partitions each image's accepted symbols: those the
runtime file's own symtab/dynsym can resolve keep attaching by name, and those
found only in the external debug file get their file offsets pre-computed and
attach via link.UprobeMultiOptions.Addresses. Offsets take symbol values from
the debug file and program headers from the runtime file -- --only-keep-debug
preserves p_vaddr but not p_offset, so using the debug file's headers puts the
probe mid-instruction (libgmp: 0x11000 too low).
Two hazards found on real distro libraries, neither reachable from the libgmp
pair the design was drawn from:
- UprobeMulti resolves the whole batch up front and fails the entire call on
the first bad entry. The x86 uprobe decoder refuses hand-written AES-NI asm
(_aesni_ctr32_ghash_6x, _aesni_ctr32_6x) with EOPNOTSUPP, and those two
symbols cut openssl from 3971 to 2185 called. attachBisect halves the batch
on error, so a bad entry drops only itself.
- openSUSE Leap 16 ships libopenssl3-debuginfo built from a different build
of libcrypto than the library on disk (780e425a... vs b844407f..., different
segment sizes, no .build-id symlink). Its symbol values describe other code,
and openssl version -a died with SIGSEGV. SymbolFileOffsets now returns
nothing unless the two files' build-ids match, so no caller can express the
unsafe pairing; Leap 16 falls back to name-only attach automatically.
attachPlan.verify re-checks the build-id against the mapped library at
attach time, catching a package upgrade between install and run.
.funcs.json carries ImageFuncs{BuildID, Names, Offsets} and still reads the
pre-0.8.4 bare array as a name-only plan. restoreLibraryBackups survives only
to clean up installs made by 0.8.3 and goes one release after 0.8.4.
test_gmp.sh is rewritten as the gate: libgmp is byte-identical across the whole
install/uninstall cycle, rpm -V is unchanged, and coverage is unchanged at
15/98. Full sweep green on the VM host and in both container images; coverage
up in all three packages (cmd 75.4 -> 77.1, shim_binary 25.3 -> 30.8, funkutil
90.9 -> 92.1).
E1: the rootPID field was written by Start and never read. imgSymbols and imgCookies, the plan's other two targets, had already been replaced by plans and scanned in D1. E2: Tracer.Stop returned an always-nil error; both call sites ignored it. E3: handleDynamicLoad runs on the readLoop goroutine, so nothing drains the ringbuf while it works, and it re-parsed the symbol tables of every library it had previously rejected on every dlopen event. The instrumented set is now named scanned and is marked before the noisy/filter/symtab checks rather than after a successful attach, so a rejected library stays rejected. Still no dedicated goroutine: kernel-side dedup plus a 256 KB ringbuf means overflow needs a pathological plugin load, and the ponytail comment names the upgrade path if one shows up. E4: every subcommand built its FlagSet with flag.ExitOnError, so fs.Parse exited the process behind main's back and every `if err := fs.Parse(...)` branch in the file was unreachable -- cmdUninstall and cmdTrace did not even have one. newFlagSet uses ContinueOnError and discards flag's own output, so a bad flag surfaces exactly once through main's uniform "<cmd> error: ..." path with exit 1 instead of flag's exit 2. -h returns flag.ErrHelp, which main answers with helpText. E5 (scale the "seen" map's dlopen headroom off the static function count) was implemented and rejected: test_nginx_dlopen.sh failed on a missing ngx_http_echo_handler, because the dlopen path instruments every newly mapped non-noisy library rather than just the plugin, and nginx's 3940 static functions bought only 1970 dynamic slots. The measurement is recorded in the constant's doc comment. The 800 KB reserve stands until a BPF_MAP_TYPE_HASH "seen" map is worth its hot-path lookup. Full sweep green: VM host binaries, dlopen/relay/symlink tests, the python suite, and both container images. cmd coverage 77.1 -> 79.1, shim_binary 30.8 -> 31.2.
With the default --formats html,txt,xml, splitCalledUncalled ran once per image per format and summarizeCoverage twice. buildReportSet does both once into a reportSet; emitReport, perImage and the three generators consume that instead of the coverage map. generateAggregateHTMLReport now clones Rows before shortening image names to their base — it used to own a fresh summarizeCoverage result, and rewriting a shared one corrupts the txt and xml output. BenchmarkEmitReportMultiFormat (new, the two-format path the item is about): 8.6 -> 7.4 ms. No other benchmark moved outside noise.
printTxtReport used fmt.Printf against os.Stdout, which Go does not buffer: one write(2) per function name. It now wraps the writer it is handed in a bufio.Writer and flushes on return, and emitReport passes os.Stdout. 5000-function corpus, report --formats txt, strace -c -e trace=write: 5015 -> 42 writes, output byte-identical.
SymtabFunctions demangled every symbol to evaluate keep, then threw the
result away; writeFunctionsLog and `enumerate` demangled the whole set
again, and the shim's dlopen path did it three times per library. It now
returns funkutil.Func{Raw, Demangled} and the demangled names travel out of
EnumerateFunctions as a second map, keyed like the first.
Not a field on ImageFuncs: that type is the .funcs.json schema, and the
display names are neither serialised nor of any use to the shim.
enumerate --no-libs /usr/lib64/libstdc++.so.6 (3986 C++ symbols): 41 -> 26 ms.
The dependency loop in EnumerateFunctions parsed each library's ELF and DWARF one at a time, so a single large dependency held up every other one behind it. Run it under an errgroup limited to GOMAXPROCS(0): each goroutine fills one preallocated slot and both result maps are merged from that slice after Wait, in libs order, so the maps are never written concurrently and the merge does not depend on completion order. Per-library errors stay per-library warnings. enumerate /usr/bin/clang-tidy-22 (7 traceable libraries, 64608 functions): 393 -> 249 ms, byte-identical output. BenchmarkEnumerateFunctions cannot show it — the sample binary's only dependencies are core system libs, which IsSystemLib drops, leaving the loop empty; interleaved runs against the parent commit overlap. Closes bottleneck C.3 of issue #123.
S1: drop the make(map[string]command, len(cmds)+14) size hint — a hand
synced magic number that had to track the alias block below it.
S2: calledLogPath becomes newLogPaths, returning {Called, Functions} built
from one timestamped stem. NewTracer takes the pair instead of deriving the
functions log by string-editing "_called.log" out of the called log.
S3: writeBuffered now reports the close error when nothing else failed. On
a full filesystem the failing write is often the one close performs, so
dropping it turned a truncated report into a successful run — that applies
to the HTML and XUnit writers too, not just writeFunctionsLog, which now
calls into it instead of repeating the create/buffer/flush dance.
This reverts commit a27a455, keeping its test. Measured, then judged not worth it: clang-tidy-22 (7 traceable libraries, libLLVM + libclang-cpp, 64608 functions) 393 -> 249 ms, ssh 12.8 -> 11.2 ms, git unchanged. That is 144 ms saved once, on a path that runs only during install and trace startup, in exchange for a goroutine fan-out whose per-library stderr warnings now race and whose merge order the code has to defend deliberately. TestEnumerateFunctions_WithLibraries stays: the dependency loop had no test at all before, and the benchmark cannot cover it because tests/sample/sample depends only on libraries IsSystemLib drops. Root plan.md gains section 4 recording the decision and the numbers, since docs/review-2026-08.md is deleted when this branch merges. Bottleneck C.3 of issue #123 is closed as decided against, not as done.
... | sort -rn | head -10 under set -euo pipefail: head closes its input after 10 lines, sort gets SIGPIPE, pipefail turns that into the pipeline's exit status, and set -e kills the whole script -- right after the (purely cosmetic) namespace list had already printed correctly. Never manifested running the script directly; reliably killed it under container scheduling, reproduced twice via bash -x. Trailing "|| true" matches the pattern already used two lines above for the same reason. Pre-dates this branch (5ebfbcf); found while running the pre-merge e2e sweep for fix/review-2026-08, unrelated to any of its phases.
test_glibc_hwcaps_resolution.sh: C4's ld.so glibc-hwcaps priority (<dir>/glibc-hwcaps/<level>/<soname> before <dir>/<soname>) was verified once, manually, against 319 host binaries, never as a regression test. Getting it wrong attaches a uprobe to an inode the process never maps -- no error, just silent 0% for that library. Uses bzip2/libbz2, a real distro pair with a plain and a v3-optimized build on disk (different sha256, same soname); asserts enumerate resolves to the exact file ldd says the process maps, then that image shows called functions post-run. Verified as a real guard, not a tautology, by forcing hwcapsSubdirs() to return nil and confirming the assertion fails with the expected message. Self-skips (not fail()) when the host/distro ships no hwcaps build of libbz2 at all -- true on both container images, unlike the bare VM. test_rust_ripgrep.sh: every existing demangling assertion in this suite is C++ (squid). demangle.Filter also handles Rust's mangling scheme; nothing exercised it before. Asserts a trait-impl name (<T as Trait>::method), a shape Itanium C++ demangling never produces, so it proves the Rust grammar is actually parsed rather than a generic "::" check coincidentally passing. Both wired into run_all_container_tests.sh, shared by test_container_ leap16.sh and test_container_tumbleweed.sh. Also added test_nss_dlopen.sh and test_pam_dlopen.sh to the container runner -- part of the VM sweep already but never wired into containers. Full container sweep re-run after all additions: 13 scripts, both distros green.
…tion) All items landed: C1-C4, D1, E1-E4 (E5 rejected), P1-P3, P4 (rejected, see plan.md #4), S1-S3. Full VM e2e sweep plus both containerized suites (leap16-e2e, tumbleweed-e2e, 13 scripts each) green, including two new tests (glibc-hwcaps resolution, Rust demangling) added while running it. plan.md #2 (ldd fork-exec elimination) is done via cmd/libdeps.go's DT_NEEDED walk -- delete it. #1 is unrelated to this review, #3 keeps its already-trimmed openQA packaging gap, #4 (P4 rejection) stays as the record of a decision this branch's own working doc can no longer carry.
This resolves a CI failure on Ubuntu 24.04 where ldd failed to find dependencies of symlinked binaries like appletviewer due to $ORIGIN relative path mismatches.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #156 +/- ##
==========================================
+ Coverage 59.79% 67.03% +7.23%
==========================================
Files 18 20 +2
Lines 1495 1805 +310
==========================================
+ Hits 894 1210 +316
+ Misses 601 595 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
1 participant
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.
This pull request refactors and modernizes the function enumeration logic in
cmd/enumerate.goand related ELF utility code, improving both accuracy and maintainability. The main changes include switching to a more robust function representation, preferring external debug files for symbol enumeration, and removing outdated or unsafe code paths. This update also prepares the codebase for future removal of deprecated behaviors related to system library modification.Function enumeration and symbol resolution improvements:
funkutil.Funcstruct that includes both mangled and demangled names, reducing redundant demangling and clarifying function handling throughout the codebase. (cmd/enumerate.go) [1] [2] [3] [4] [5] [6] [7].symtabsections over the main binary's, ensuring more complete symbol coverage—especially for stripped binaries or shared libraries. (cmd/enumerate.go)cmd/enumerate.go) [1] [2] [3]Deprecation and cleanup of unsafe or obsolete behaviors:
mergeLibraryDebugInfofunction and related code that previously modified system libraries in place, replacing it with a stub and comments marking it for future removal. (cmd/elfutil.go)lddand regular expressions, in favor of a safer, internal resolution mechanism. (cmd/enumerate.go) [1] [2]General code quality and maintainability:
funkutil.BuildID, removing local duplication and unnecessary imports. (cmd/elfutil.go) [1] [2] [3]cmd/enumerate.go,cmd/funkoverage.go) [1] [2] [3]