Repository navigation
Commit b0366ad
committed
Merge #7773: ci: fail the build when an executable embeds a standard library header path
b6b9b75 ci: run the stdlib header path check on the linux64 and mac builds (UdjinM6)
4471e14 guix: fail the build when a binary embeds a stdlib header path (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
A source location captured inside a standard library header is wrong twice over: `gsl::details::terminate()` prints it, so a failed precondition names a standard library internal instead of the code that broke it, and the recorded path belongs to the toolchain, so on darwin it comes from the macOS SDK - whose location differs between builders and makes the release binaries irreproducible.
Nothing catches that today. 966f532 introduced exactly this bug in `evo/creditpool.cpp` and it went unnoticed until guix attestation for v24.0.0-rc.1 disagreed on the macOS hashes: just over seven days after the commit landed, and only because one builder happened to keep their SDK somewhere other than the rest. Comparing hashes across builders is a poor detector. It needs a second builder, a full release build, and a difference in their setups before it fires at all, and it fires at the worst possible moment.
## What was done?
A check that fires on a single build instead. `contrib/guix/stdlib-path-check.py` parses each executable with LIEF and fails if one embeds a C++ standard library header path, wired as a `check-stdlib-paths` make target beside the existing `check-symbols` and `check-security`, and invoked from the guix build.
It also runs in regular CI, behind `RUN_STDLIB_PATH_CHECK`, enabled for the `linux64` and `mac` targets. `check-symbols` could not be run there - it caps GLIBC at 2.31 (`contrib/guix/symbol-check.py:31-38`) while a native CI build links against the container's far newer glibc, so it asserts a property of release artifacts that a native build legitimately violates - and `check-security` is simply not wired into regular CI today. This check has no such dependence: outside the instrumented builds noted below, an embedded standard library header path is wrong regardless of host or toolchain. Regular CI builds from a `make distdir` tree (`ci/dash/build_src.sh:29-37`), so the script is listed in `BIN_CHECKS`/`EXTRA_DIST`.
Both targets matter, and neither is redundant. On libc++ the relocation form of this bug is invisible, because clang proves the moved-from pointer non-null and drops the branch that keeps the string alive; gcc does not. On libstdc++ it is visible. Conversely the original rc.1 form - a raw pointer forwarded into the standard library - is visible on both. So darwin alone cannot cover this class, and `linux64` should not be dropped on the assumption that it does.
Only sections that hold string literals are examined: `.rodata`, `.rdata`, `__cstring`, `__const`, and anything with those prefixes, so ELF variants such as `.rodata.str1.1` are included. DWARF legitimately names standard library headers for inlined template code, and skipping debug sections by name is not portable - PE stores a long section name as an offset into the string table, so they read as `/81` rather than `.debug_*`. A first version denied those by name and drowned the mingw build in thousands of DWARF false positives. Naming the sections we want cannot pick up debug data by accident. The trade is deliberate: a literal in an unusual section would be missed, which for a gate that blocks builds is the safer direction.
Each hit reports its section, how many times the path occurs, and the first few virtual addresses, so a real literal is distinguishable at a glance from debug data, and can be attributed to the referencing code with `objdump` without a rebuild. A file LIEF cannot parse is also a failure rather than a silent pass.
`RUN_STDLIB_PATH_CHECK` defaults to false, and only `linux64` and `mac` opt in. That is a default rather than a hard exclusion - a target that inherited the variable as true would run the check - but no target sets it today. Sanitizer and fuzz builds are deliberately not opted in: their instrumentation records source locations for its own diagnostics and is expected to name standard library headers legitimately, so enabling it there would report instrumentation as a defect. They can be revisited once we know what they actually embed.
## How Has This Been Tested?
The check found a real second bug on its first run, which is the strongest evidence for it. After #7760 the linux release binaries still embedded `bits/stl_construct.h`; that turned out to be a separate defect in `gsl::not_null`, fixed in #7772, which source review had missed.
Against real artifacts:
- the pre-fix guix build of 0f87636 fails on `dashd`, `dash-qt` and `test_dash` - precisely the three binaries that differed between builders on rc.1 - and passes on `dash-cli`, `dash-tx`, `dash-util` and `dash-wallet`, precisely the four that were byte-identical
- discrimination was checked directly: an object file containing twelve standard library paths in total reports exactly one, the string literal in `__TEXT,__cstring`
- no false positives on an ordinary local non-guix build of `dashd`, `test_dash`, `dash-qt` and `dash-cli`
The two halves were separated across branches so each can be observed on its own, since this PR on its own is expected to fail until #7772 merges:
- [`check-stdlib-paths`](https://github.com/UdjinM6/dash/commits/refs/heads/check-stdlib-paths/) - this PR, the check without the fix. Regular CI `linux64` and the guix libstdc++ hosts fail on `/usr/include/c++/13/bits/stl_construct.h`; `mac` and both guix darwin hosts pass. That asymmetry is the coverage point above: clang drops the branch that keeps the string alive, gcc does not.
- [`check-stdlib-paths-with-fix`](https://github.com/UdjinM6/dash/commits/refs/heads/check-stdlib-paths-with-fix/) - this PR rebased on top of #7772, kept purely as evidence and not proposed for merge. Every guix host passes - `x86_64-linux-gnu`, `riscv64`, `aarch64`, `powerpc64`, `x86_64-w64-mingw32` and both darwin - along with `linux64` and `mac` in regular CI.
Taken together: the check fails on a tree with the bug, passes on a tree with the fix, and produces no false positives across four object-format and standard-library combinations.
## Breaking Changes
None at runtime. This adds a build-time check; a build that would have produced a binary embedding such a path now fails instead.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Top commit has no ACKs.
Tree-SHA512: b4941bfb9627910a4152b7b836a1bc92031eeecd5e38bc1765c89dc6509f570933e681193b106461bad01afb560e5ace939c10b0dbc38785aaf8ccf7b44e2a448 files changed
Lines changed: 117 additions & 1 deletion
File tree
- ci
- dash
- test
- contrib/guix
- libexec
- src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
55 | 55 | | |
56 | 56 | | |
57 | 57 | | |
| 58 | + | |
58 | 59 | | |
59 | 60 | | |
60 | 61 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
47 | 47 | | |
48 | 48 | | |
49 | 49 | | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
50 | 57 | | |
51 | 58 | | |
52 | 59 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
39 | 39 | | |
40 | 40 | | |
41 | 41 | | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
42 | 47 | | |
43 | 48 | | |
44 | 49 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
16 | 16 | | |
17 | 17 | | |
18 | 18 | | |
| 19 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
287 | 287 | | |
288 | 288 | | |
289 | 289 | | |
| 290 | + | |
| 291 | + | |
290 | 292 | | |
291 | 293 | | |
292 | 294 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
159 | 159 | | |
160 | 160 | | |
161 | 161 | | |
162 | | - | |
| 162 | + | |
163 | 163 | | |
164 | 164 | | |
165 | 165 | | |
| |||
1495 | 1495 | | |
1496 | 1496 | | |
1497 | 1497 | | |
| 1498 | + | |
| 1499 | + | |
| 1500 | + | |
| 1501 | + | |
1498 | 1502 | | |
1499 | 1503 | | |
1500 | 1504 | | |
| |||
0 commit comments