Skip to content

fix: report the real source location for GetCreditPool's not_null precondition - #7760

Merged
PastaPastaPasta merged 1 commit into
dashpay:developfrom
UdjinM6:fix-macos-sdk-path-reproducibility
Sep 28, 2026
Merged

PastaPastaPasta merged 1 commit into
dashpay:developfrom
UdjinM6:fix-macos-sdk-path-reproducibility

Conversation

@UdjinM6

@UdjinM6 UdjinM6 commented Sep 27, 2026 •

Copy link
Copy Markdown

Issue being fixed or feature implemented

CCreditPoolManager::GetCreditPool() records the wrong source location for its not_null precondition, so a failure blames a standard library header instead of the code that produced the null pointer. This affects every platform, and on darwin it additionally makes the release binaries irreproducible.

966f532, from #7693, changed evo/creditpool.cpp to call emplace() rather than push() on a std::stack<gsl::not_null<const CBlockIndex*>>, to satisfy clang-tidy's modernize-use-emplace. gsl::not_null's converting constructor takes nostd::source_location loc = nostd::source_location::current() (src/gsl/pointers.h:102), backed by __builtin_FILE(), and forwards it to Expects(). A default argument is evaluated at the point of the call: push() constructs the not_null at the call site in creditpool.cpp, while emplace() forwards the raw pointer and constructs it inside the standard library's construction helper.

That location is not merely stored, it is printed. gsl::details::terminate() (src/gsl/assert.cpp:26-35) writes file_name(), line(), column() and function_name() to stderr and to debug.log via LogPrintf before calling std::terminate(), in release builds as well as debug ones. So since that commit, a null pprev reaching GetCreditPool reports a standard library internal rather than the loop that produced it — __memory/construct_at.h with libc++, bits/stl_construct.h with libstdc++. The linux release binaries name /usr/include/c++/bits/stl_construct.h, a path that exists on no user's machine.

modernize-use-emplace is a false positive whenever the element type's constructor captures the caller's location this way.

The reproducibility symptom is what led here. On darwin the libc++ headers resolve through -isysroot$(OSX_SDK) (depends/hosts/darwin.mk:75-81), and contrib/guix/guix-build:452 shares the SDK into the container at the builder's own path — --share="$SDK_PATH", with no =/target remap, unlike --share="$PWD"=/dash. The embedded string therefore differs between builders, and guix attestation for v24.0.0-rc.1 disagreed on the macOS hashes for dashd, dash-qt and test_dash while every Linux, Windows and source output matched (dashpay/guix.sigs#190 vs #191).

Worth stating plainly, since it is the obvious question: the non-darwin binaries carry the same wrong location. They just carry it at a path the guix build already normalises (contrib/guix/libexec/build.sh:221 rewrites /gnu/store/... to /usr), so it is identical for every builder and comparing hashes could never have surfaced it. The leak was invisible on those platforms, not absent. It also predates no release: the commit landed 2026-09-18 and v24.0.0-rc.1 was tagged 2026-09-24, so this is the first release that ever contained it.

What was done?

Reverted the emplace() to push(), with a NOLINT(modernize-use-emplace) and a comment recording why, so the tidy rewrite is not reapplied. gsl::not_null is a thin wrapper over a raw pointer, so both forms emit the same store; emplace() bought nothing here beyond silencing the check.

How Has This Been Tested?

No test was added. Observing the logged location requires the precondition to fail, which means a null pprev — a state that would mean the block index is already corrupt.

Verified against the real build artifacts instead. The guix build of the parent commit (0f87636) embeds the standard library path in exactly dashd, dash-qt and test_dash, and in none of dash-cli, dash-tx, dash-util or dash-wallet — matching the three-versus-four split that was observed between builders on rc.1. The guix build of this branch embeds it in none of them, on both darwin and linux. https://github.com/UdjinM6/dash/actions/runs/36436424685

Also audited the tree for the same pattern elsewhere. gsl::not_null appears in 145 places outside src/gsl, all parameters or locals except two stored ones: the stack fixed here, and UtilParameters::m_base_index (src/llmq/utils.h:36). Every UtilParameters construction site is braced aggregate initialisation at the call site, so none forwards a raw pointer through a standard library header. src/gsl is also the only user of nostd::source_location in the tree.

Breaking Changes

None. No consensus, P2P, RPC, wallet or on-disk behaviour changes. The only difference is the file, line and function reported if the not_null precondition in GetCreditPool ever fires.

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

@UdjinM6 UdjinM6 added this to the 24 milestone Sep 27, 2026
@thepastaclaw

thepastaclaw commented Sep 27, 2026 •

Copy link
Copy Markdown
Collaborator

⚠️ Automated review could not complete (commit 00aabf5)
Reason: PR is closed (merged)

@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: e913c5fe-3d48-44d0-911c-05bad0b03718

📥 Commits

Reviewing files that changed from the base of the PR and between df48b91 and 00aabf5.

📒 Files selected for processing (1)
  • src/evo/creditpool.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


Walkthrough

GetCreditPool now adds the block index with push instead of emplace. A comment describes the gsl::not_null construction behavior and the reason for the NOLINT annotation.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~3 minutes

Merge Risk: ⚪ Minimal · up to 00aab

This change improves the location reported for a null-precondition failure without changing the stored pointer behavior. No PR-introduced merge risk was identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to ba1fa

The changes appear to preserve credit-pool validation while addressing builder-specific paths in macOS binaries. No new security exposure was identified, but the resulting release binaries were not available to verify the reproducibility outcome.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The build-path change reaches Darwin outputs compiled through these commands. The credit-pool insertion change does not add a caller or expand the block-validation entrypoint.

Trust Boundaries and Controls

  • observed — The block-index loop still checks for a null pointer before insertion, and gsl::not_null still checks the pointer during construction. The changed insertion does not remove those controls.

Hardening Proposals

  • proposed — Compare independently built Darwin release binaries to confirm that the path mapping removes the reported builder-dependent output difference.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely identifies the main change: reporting the correct source location for the GetCreditPool not_null precondition.
Description check ✅ Passed The description directly explains the incorrect source location, the push() change, the testing performed, and the scope of the fix.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@knst

knst commented Sep 27, 2026

Copy link
Copy Markdown
Collaborator

why do we need to replace emplace() to push() if sdk path is fixed now? @UdjinM6

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Phase 1 + Phase 2

Verified both commits against exact head ba1fafa and found no actionable defects. Source inspection confirms that push restores call-site construction without changing credit-pool calculations, and that the SDK prefix mapping applies to cross-compilation while preserving native macOS overrides; an independently compiled four-case probe reproduced the reported SDK-path leakage and confirmed both fixes. The diff passes whitespace checks; full Guix release reproducibility was not independently tested.

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: dash-core-commit-history); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

  • Triage: low by gpt-6-astra (effort low) — The diff is small and contained, replacing emplace with push to preserve caller source locations and adding a macOS SDK prefix-map build flag without changing credit-pool logic or consensus behavior.
  • Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer, glm-5.3-flash — dash-core-commit-history (completed, effort high); agent phase1-reviewer
  • Phase 1 model: glm-5.3-flash — zai quota: 5h 97% left, weekly 34% left; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 13% left, 5h 100% left)
  • Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
  • Phase 2 reviewers: gpt-6-astra — general (completed, effort medium); agent phase2-reviewer, gpt-6-astra — dash-core-commit-history (completed, effort medium); agent phase2-reviewer

@thepastaclaw thepastaclaw added the pastaclaw:approved thepastaclaw's latest review approved this PR label Sep 27, 2026
@UdjinM6

UdjinM6 commented Sep 27, 2026

Copy link
Copy Markdown
Author

why do we need to replace emplace() to push() if sdk path is fixed now?

  1. The location is printed, not just embedded. gsl::details::terminate() (src/gsl/assert.cpp:26) writes loc.file_name(), line(), column() and function_name() to stderr and to debug.log via LogPrintf before calling std::terminate() — in release builds, not just debug ones. So with emplace(), a null pprev reaching GetCreditPool reports:

    ERROR: error detected null not_null detected at .../__memory/construct_at.h:41:...:construct_at

    instead of creditpool.cpp:260 / GetCreditPool. -ffile-prefix-map does NOT repair that. It makes the wrong location look tidier — /macos-sdk/usr/include/c++/v1/__memory/construct_at.h.

  2. The flag only covers the depends cross-build. depends/builders/darwin.mk:13-14 overrides darwin_CC/darwin_CXX when building natively on macOS, so a developer building on a Mac still gets /Applications/Xcode.app/... baked in. The remap protects the Guix release path; it does NOT remove the leak at its source.

  3. emplace() also buys nothing here. gsl::not_null is a thin wrapper over a raw pointer, so push() of an implicitly converted temporary and emplace() of the pointer emit the same store. modernize-use-emplace is simply a false positive whenever the element type's constructor captures the caller's location via a defaulted source_location.

I would drop the second commit if you'd rather keep the diff minimal — the first one is the actual fix:

variant location recorded in the binary varies per builder?
emplace(), no flag (current develop) /Applications/Xcode.app/…/MacOSX.sdk/usr/include/c++/v1/__memory/construct_at.h yes — this is the rc.1 divergence
push(), no flag (first commit alone) the call site in creditpool.cpp no
emplace() + flag (second commit alone) /macos-sdk/usr/include/c++/v1/__memory/construct_at.h no, but still the wrong location
both (this PR) the call site in creditpool.cpp no

@UdjinM6
UdjinM6 force-pushed the fix-macos-sdk-path-reproducibility branch from ba1fafa to df48b91 Compare September 27, 2026 18:01
@thepastaclaw thepastaclaw removed the pastaclaw:approved thepastaclaw's latest review approved this PR label Sep 27, 2026
@UdjinM6 UdjinM6 changed the title fix: stop embedding the builder's macOS SDK path in release binaries fix: keep not_null construction at the call site in GetCreditPool Sep 27, 2026
@UdjinM6

UdjinM6 commented Sep 27, 2026

Copy link
Copy Markdown
Author

Dropped ba1fafa. After investigating a bit more I think fixing the SDK mount Pasta proposed originally is a more focused approach but it belongs to a separate guix-only PR.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

Verified the complete diff at df48b91 and found no actionable defects. The change preserves credit-pool traversal and accounting while constructing not_null at the application call site; a local C++20 probe using the repository's GSL headers confirmed the source-location difference between push and emplace. The diff whitespace check passed; full project builds and test suites were not run.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: dash-core-commit-history); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

  • Triage: low by gpt-6-astra (effort low) — The change is a small, contained replacement of emplace() with push() in GetCreditPool to preserve not_null diagnostic source locations, with a lint suppression and explanatory comment, without changing credit-pool or consensus logic.
  • Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer, glm-5.3-flash — dash-core-commit-history (completed, effort high); agent phase1-reviewer
  • Phase 1 model: glm-5.3-flash — zai quota: 5h 93% left, weekly 27% left; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 13% left, 5h 100% left)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
  • Phase 2 reviewers: gpt-6-astra — general (completed, effort medium); agent phase2-reviewer, gpt-6-astra — dash-core-commit-history (completed, effort medium); agent phase2-reviewer, gpt-6-astra — general (completed, effort medium); agent phase2-reviewer, gpt-6-astra — dash-core-commit-history (completed, effort medium); agent phase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

@thepastaclaw thepastaclaw added the pastaclaw:approved thepastaclaw's latest review approved this PR label Sep 27, 2026
@UdjinM6
UdjinM6 force-pushed the fix-macos-sdk-path-reproducibility branch from df48b91 to 4989a84 Compare September 28, 2026 14:25
@UdjinM6 UdjinM6 changed the title fix: keep not_null construction at the call site in GetCreditPool fix: report the real source location for GetCreditPool's not_null precondition Sep 28, 2026
@thepastaclaw thepastaclaw removed the pastaclaw:approved thepastaclaw's latest review approved this PR label Sep 28, 2026
…condition

966f532 turned `to_calculate.push(block_index)` into
`to_calculate.emplace(block_index)` to satisfy clang-tidy's
modernize-use-emplace. `to_calculate` holds
`gsl::not_null<const CBlockIndex*>`, whose converting constructor takes
`nostd::source_location loc = nostd::source_location::current()`
(src/gsl/pointers.h) and forwards it to `Expects()`. A default argument is
evaluated at the point of the call, so `push()` evaluates `__builtin_FILE()`
here in creditpool.cpp, while `emplace()` forwards the raw pointer and
evaluates it inside the standard library's construction helper.

That location is not merely stored, it is printed. `gsl::details::terminate()`
(src/gsl/assert.cpp) writes `file_name()`, `line()`, `column()` and
`function_name()` to stderr and to debug.log before calling
`std::terminate()`, in release builds as well as debug ones. Since the
refactor, a null `pprev` reaching `GetCreditPool` therefore names a standard
library internal rather than the loop that produced it, on every platform:
`__memory/construct_at.h` with libc++, `bits/stl_construct.h` with libstdc++.

On darwin it also breaks reproducibility. The libc++ headers resolve through
`-isysroot$(OSX_SDK)` (depends/hosts/darwin.mk) and contrib/guix/guix-build
shares the SDK into the container at the builder's own path, so the embedded
string differs between builders. That is how this was found: guix attestation
for v24.0.0-rc.1 disagreed on the macOS hashes for dashd, dash-qt and test_dash
while every non-darwin output matched. Linux binaries carry the same wrong
location, but at a path the guix build already normalises, so comparing hashes
could never have surfaced it there.

modernize-use-emplace is a false positive whenever the element type's
constructor captures the caller's location, so suppress it here rather than
let the refactor be reapplied.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@UdjinM6
UdjinM6 force-pushed the fix-macos-sdk-path-reproducibility branch from 4989a84 to 00aabf5 Compare September 28, 2026 14:29

@knst knst left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 00aabf5

@PastaPastaPasta
PastaPastaPasta merged commit 3ba0805 into dashpay:develop Sep 28, 2026
43 of 44 checks passed
PastaPastaPasta added a commit that referenced this pull request Sep 30, 2026
…is relocated

be3c436 fix: don't re-capture a source_location when a not_null is relocated (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented

  Relocating a `gsl::not_null` re-records where it was moved, so a failed precondition names whatever code moved it rather than the code that produced the null pointer. When the relocation happens inside a standard library header, that header's path is embedded in the shipped binaries.

  `not_null`'s converting constructor takes a Dash-specific defaulted `nostd::source_location loc = nostd::source_location::current()` (`src/gsl/pointers.h:122-126`) and forwards it to `Expects()`. Upstream Microsoft GSL has no such parameter, so while its own overload resolution behaves the same way, there is nothing there to recapture. The other candidates were `not_null(const not_null&)` and the templated `not_null(const not_null<U>&)`. For an rvalue or a non-const lvalue, the forwarding template binds `U&&` exactly while both of those need an added `const`, so overload resolution picked the template, and every relocation re-evaluated `source_location::current()` and re-ran the null check on a value already known to be non-null.

  That location is printed, not merely stored: `gsl::details::terminate()` (`src/gsl/assert.cpp:26-37`) writes `file_name()`, `line()`, `column()` and `function_name()` to stderr and to `debug.log` before `std::terminate()`, in release builds as well as debug ones.

  `std::stack<gsl::not_null<const CBlockIndex*>>` in `evo/creditpool.cpp` moves its element into a freshly allocated deque node, so the linux release binaries embed `/usr/include/c++/13/bits/stl_construct.h` and would report it as the origin of a failed precondition. Confirmed by symbolizing the only reference to that string in a release `dashd`: it resolves through `_M_push_back_aux` and `_M_allocate_node` to a `source_location` built with file `stl_construct.h` and line 97, followed by the null-check branch. The darwin binaries are clean only because clang drops the unreachable `terminate()` path that keeps the string alive; gcc does not.

  This is the remaining half of #7760. That PR stopped a raw pointer being forwarded into the standard library and converted there; this stops an already-constructed `not_null` re-capturing when a container relocates it. Neither subsumes the other, and with only #7760 the linux binaries still carried the path.

  ## What was done?

  Excluded every `not_null` specialisation from the location-capturing constructor, through a `details::is_not_null` trait rather than `is_same`, so that converting between specialisations does not select it either. Relocation then binds to the copy constructor.

  No move constructor is added, deliberately. Moving would leave a smart-pointer `T` null and break the invariant the type exists to enforce, which is why upstream omits one and why `strict_not_null`'s own move can be defaulted - it copies the base rather than moving it, as its comment at `src/gsl/pointers.h:333-335` says.

  The `not_null<U>` to `not_null<T>` constructor no longer delegates, which recorded this header as the origin of any failure, but keeps its check: `U` is non-null, yet nothing constrains a `U` to `T` conversion to preserve that. It takes its own location so the check names the caller.

  Two assertions pin the result. The first fails if the location-capturing constructor is selected for a relocation again, since as written it is not `noexcept` - it would not catch one that someone explicitly declared `noexcept`, which is a smaller hole than leaving it unguarded. The second fails if a usable move constructor is added, which the first would not catch, since a defaulted move constructor satisfies it too; a move-only `T` that must not be movable is what detects one.

  Deliberately not addressed, to keep this to the defect: `make_not_null` and the `strict_not_null` constructors still capture locations inside `pointers.h`, and `strict_not_null` is not covered by the trait because it derives from `not_null` rather than being a specialisation. Neither embeds a standard library path, and `strict_not_null` is unused outside `src/gsl/`.

  ## How Has This Been Tested?

  No unit test was added. The captured location is observable only by making the precondition fail, which needs a death or subprocess test around a deliberately null pointer; that seemed a poor trade for the compile-time guarantee below. The two `static_assert`s are the regression test, and both were checked in each direction: they pass against this header, and fail against `develop`'s header and against a header with a move constructor re-added.

  Verified by compiling probes that use `#line` to attribute a relocation to a synthetic header:

  - before: moving a `not_null`, copying a non-const lvalue, and converting `not_null<B*>` to `not_null<const B*>` all recorded the mover's file
  - after: all three record only the genuine construction site, and a 600-element `std::deque` forcing node allocation embeds no standard library path at all
  - a `not_null<std::shared_ptr<int>>` was moved and the source then checked at runtime: still non-null, so the invariant holds

  Full build plus the `evo_*` and `llmq_*` suites (228 cases) pass locally. The guix build passes on all seven hosts - `x86_64-linux-gnu`, `riscv64`, `aarch64`, `powerpc64`, `x86_64-w64-mingw32` and both darwin - which is the real coverage here, since only two of those toolchains are reachable locally.

  ## Breaking Changes

  None. No consensus, P2P, RPC, wallet or on-disk behaviour changes, and no change to the class layout - the sole data member is still `T`. The `not_null<U>` to `not_null<T>` constructor gains a defaulted parameter, and overload selection among the inline constructors changes, so this is not source-identical for anything that named those overloads explicitly. For the copyable `T` used in this tree, raw pointers and `shared_ptr`, relocation already copied the pointer; it simply no longer re-runs a redundant check or records a location while doing so.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [ ] 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: d24e899cbd1376d2482cc7d8f302b1f550c375a40e34e513a1e5da1d1090265aac1f1f314c72fe45ed2aee5cdcc27dd8ebbe60092a5792e56e0c69ee6ce1f0b1
PastaPastaPasta added a commit that referenced this pull request Sep 30, 2026
…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: b4941bfb9627910a4152b7b836a1bc92031eeecd5e38bc1765c89dc6509f570933e681193b106461bad01afb560e5ace939c10b0dbc38785aaf8ccf7b44e2a44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants