fix(memory): source forwarded env below memtrack's capability boundary#439
Conversation
Merging this PR will not alter performance
|
Greptile SummaryThis PR fixes memory benchmark environment forwarding across the memtrack capability boundary. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "fix(memory): source forwarded env below ..." | Re-trigger Greptile |
ec0a5c8 to
1f71d95
Compare
GuillaumeLagrange
left a comment
There was a problem hiding this comment.
OLGTM
Key question though: why did we not catch it before? Is there an integration test that we are missing?
We had a test which tested forwarding the PATH env var, but LD_* env vars are handled differently |
codspeed-memtrack holds file capabilities, so a non-root user executing it enters glibc secure-execution mode (AT_SECURE=1), which strips LD_* variables from the environment before main(). The memory executor sourced the forwarded environment around memtrack, so LD_LIBRARY_PATH was restored and then stripped again before the spawned benchmark could inherit it. Benchmarks relying on LD_LIBRARY_PATH (e.g. neqo loading a locally built NSS) fell back to the system library and panicked with UnsupportedVersion. Source the env inside the command memtrack runs via `bash -c`, matching how walltime already layers privilege escalation outside the env re-source. Add prefix_command_with_env for the raw `source <file> && <cmd>` snippet and reuse it from wrap_with_env.
1f71d95 to
6bf0d30
Compare
Root cause
codspeed-memtrackholds file capabilities (cap_bpf,cap_perfmon,cap_sys_admin). A non-root user executing a capability binary enters glibc secure-execution mode (AT_SECURE=1), which stripsLD_*variables from the environment beforemain().The memory executor sourced the forwarded environment around memtrack:
So
LD_LIBRARY_PATHwas restored and then stripped a second time, before the benchmark memtrack spawns could inherit it.Walltime is unaffected because it layers privilege escalation (
sudo/perf/systemd scope) outside the env re-source, keepingsourceat the innermost layer.Fix
Source the env inside the command memtrack runs via its own
bash -c, below the capability boundary:prefix_command_with_envfor the rawsource <file> && <cmd>snippet (no outerbash -c) and reuse it fromwrap_with_env.MemoryExecutor::build_memtrack_commandnow prefixes the bench command and passes it as memtrack's argument; the outer bash wrapper is gone.Test
test_memory_executor_forwards_ld_library_path(Linux +GITHUB_ACTIONSgated) exercises this end-to-end. The existing..._forwards_pathtest never caught it becausePATHis not anLD_*variable.Note
If the memory test suite is ever run as root, memtrack's caps add nothing, secure-exec isn't triggered, and the test would pass even with the bug present, same limitation as the real-world scenario, which only affects the non-root
runneruser.Verification
Reproduced the mechanism with a minimal capability binary that runs its argument via
bash -c(memtrack's exact structure), as a non-root user:LD_LIBRARY_PATH/custom/lib✅