#36677 util: Make files opened with fdbridge::fopen noninheritable

ReviewabilityReviewsAgreementSize
#36677 SirECMG util: Make files opened with fdbridge::fopen noninheritableReady0Strong47 + 41 tests
  • Prevent file descriptor leaks into child processes like external signers or notification scripts
  • Ensure files opened across platforms are marked non-inheritable by default
  • Ready to review
  • No review verdicts recorded
  • Substantive support with all prior objections resolved
  • Verified on Windows that opened files lack inheritance flags (ValeraFinebits)
  • Initial race condition concern in multithreaded file opening was addressed (hebasto)
  • 47 lines added or modified outside tests
  • 41 lines added or modified in tests
  • 4 lines removed in total
  • 2 files, 2 commits
Utilities (logging, arguments, libraries)#22 of 59
P3 · bug fix
  • P3 because it hardens shared file opening primitives against descriptor leaks into child processes
  • Worthwhile security hygiene for external tool execution but deferrable without active failures
  • Ranking pass: Moved up: descriptor leaks into spawned signer/notification processes are a security-hygiene issue in a shared file primitive.
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Marks files opened through fsbridge::fopen as non-inheritable and close-on-exec across platforms. It appends the 'e' mode specifier on Linux, appends 'N' on Windows, and implements an open() fallback with O_CLOEXEC followed by fdopen() for other POSIX operating systems like macOS and BSD. A unit test is included to assert that opened files lack the inheritance flag.

Problem: File descriptors and handles opened through fsbridge::fopen remain inheritable across process boundaries by default, risking descriptor leaks into child processes executed by Bitcoin Core (such as external signers or notification scripts).

Evidence of importance:

  • Addresses issue #36668 ('util: Make files opened via `fsbridge::fopen()` not inheritable by child processes').

Risk of a mistake: low (other, tests). A mistake in the fopen wrapper or mode parsing could cause file open failures across utility callers on non-Linux POSIX systems.

Changed since the previous assessment: The pull request is now linked to issue #36668.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • reviewer: hebasto to re-review the revised implementation addressing race condition concerns 2026-10-28

Agreement

Strong

substantive support, no open objection (ValeraFinebits)

Thread notes: winterrdog commented suggesting to add Fixes #36668 to the PR description to auto-close the issue on merge; this is a procedural comment and introduces no claims.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
hebasto (member)question, correctnessIs the 'e' mode flag in fopen supported across all non-Windows systems?answered · author changed implementation into three cases addressing Linux, Windows, and POSIXansweredyes2026-10-27: 'Is "e" available on all non-Windows systems?'
Status from: 2026-10-28: SirECMG: 'I changed implementation into three cases'
ValeraFinebits (none)objection, correctnessCompilation errors in fs_tests.cpp prevent test_bitcoin from building on Windowsfixed · fix pushed · tests now build and pass on Windowschangedno2026-10-28: 'The compilation errors in fs_tests.cpp prevent test_bitcoin from building on Windows.'
Status from: 2026-10-28: ValeraFinebits: 'Tested on Windows at 0c9ab56: the new test passes with the fix'
ValeraFinebits (none)suggestion, maintenanceAdd <cstring> for std::strchrfixed · fix pushed · author added the missing includeansweredyes2026-10-28: 'Could we also add <cstring> for std::strchr?'
Status from: 2026-10-28: SirECMG: 'Added the missing include!'
hebasto (member)objection, safetyConsecutive fopen and fcntl(FD_CLOEXEC) calls are not atomic and race in multithreaded processesfixed · fix pushed · author implemented separate Linux/Windows/POSIX paths to avoid the non-atomic racechangedyes2026-10-28: 'Unfortunately, the consecutive fopen and fcntl(..., FD_CLOEXEC) calls are not atomic, which is a race in a multithreaded process.'
Status from: 2026-10-28: SirECMG: 'I changed implementation into three cases'
ValeraFinebits (none)suggestion, approachKeep 'e' on Linux and use open with O_CLOEXEC followed by fdopen on macOS/POSIXfixed · fix pushed · implemented by author across platform-specific branchesansweredyes2026-10-28: 'Could we keep the original e approach on Linux and handle macOS separately with open using O_CLOEXEC, followed by fdopen?'
Status from: 2026-10-28: SirECMG: 'I changed implementation into three cases'

Support:

  • ValeraFinebits (none) (2026-10-28) [tested]: Verified on Windows that the test passes with the fix and fails without it on HANDLE_FLAG_INHERIT

Participants:

  • hebasto (member, objection, 2 statements 2026-10-27 to 2026-10-28): objected to non-atomic fopen and fcntl race condition in multithreaded processes, and questioned 'e' support on POSIX
  • ValeraFinebits (none, support, 7 statements 2026-10-28 to 2026-10-28): tested on Windows, suggested implementation improvements and fixes for macOS/POSIX
  • winterrdog (contributor, neutral, 2 statements 2026-10-28 to 2026-10-30): requested squashing commits and suggested adding Fixes to PR description

Files

41 lines under test/bench/ci.

  • src/util/fs.cpp +47/-4
  • src/test/fs_tests.cpp +41/-0

Card

The short text the ranking pass compares across PRs.

Makes files opened with fsbridge::fopen non-inheritable across process boundaries by appending 'e' on Linux, 'N' on Windows, and falling back to open() with O_CLOEXEC and fdopen() on other POSIX platforms. This prevents file descriptors from leaking into child processes spawned for external signers or notification hooks. The change addresses issue #36668 and includes unit tests checking the handle flags on Windows and POSIX.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-31T01:18, confidence high, input hash 7c266c259d020394
thread read through 2026-10-30 (16 statements), head 17ea8dd818
labels: Utils/log/libs

Categories

Utilities (logging, arguments, libraries)

P3 · bug fix (ranking pass: Moved up: descriptor leaks into spawned signer/notification processes are a security-hygiene issue in a shared file primitive.)

Prevents file descriptor leaks by ensuring fsbridge::fopen sets close-on-exec/non-inheritable flags across platforms. The problem description notes handles 'remain inheritable across process boundaries by default, risking descriptor leaks into child processes executed by Bitcoin Core (such as external signers or notification scripts)'. This is worthwhile security hygiene and robustness for utility primitives, but deferrable as it is not actively causing critical failures.

Membership: Matches the 'Utils/log/libs' label and edits src/util/fs.cpp to harden shared filesystem wrapper primitives.

Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 0