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

pull SirECMG wants to merge 2 commits into bitcoin:master from SirECMG:u/sirecmg/36668-fopen-inheritable-fix changing 2 files +88 −4
  1. SirECMG commented at 6:51 PM on October 27, 2026: none

    Use the 'e' mode on POSIX to set close-on-exec and the 'N' mode on windows to prevent file handle inheritance by child processes.

    This PR is still in WIP... until I add some test cases and verify that it works on Windows and Mac. I've only tested it on Linux (OpenSUSE Tumbleweed)

    <!-- *** Please remove the following help text before submitting: *** Pull requests may be closed immediately if they: - do not have a rationale and clear improvement - do not adhere to doc/AI_POLICY.md GUI-related pull requests should be opened against https://github.com/bitcoin-core/gui first. See CONTRIBUTING.md -->

    <!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

    <!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

  2. DrahtBot added the label Utils/log/libs on Oct 27, 2026
  3. DrahtBot commented at 6:51 PM on October 27, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    External sites

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #36620 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36620.svg"></sub> (test: Add case for libstdc++ bug on Windows by hebasto)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. SirECMG force-pushed on Oct 27, 2026
  5. in src/util/fs.cpp:26 in c354037d44 
      22 | @@ -23,9 +23,11 @@ namespace fsbridge {
  23 |  FILE *fopen(const fs::path& p, const char *mode)
  24 |  {
  25 |  #ifndef WIN32
  26 | -    return ::fopen(p.c_str(), mode);
  27 | +    const std::string safe_mode(std::string{mode} + "e");
    hebasto commented at 9:20 PM on October 27, 2026:

    Is "e" available on all non-Windows systems?

    ValeraFinebits commented at 1:49 PM on October 28, 2026:

    Could we keep the original e approach on Linux and handle macOS separately with open using O_CLOEXEC, followed by fdopen?

  6. DrahtBot added the label CI failed on Oct 27, 2026
  7. DrahtBot commented at 10:25 PM on October 27, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task macOS native: https://github.com/bitcoin/bitcoin/actions/runs/40583887664/job/122476509348</sub> <sub>LLM reason (✨ experimental): CI failed because fs_tests/fsbridge_fopen_noninheritable failed its check that the file descriptor has the close-on-exec flag set.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  8. in src/util/fs.cpp:27 in ca864d2fc1 outdated 
      22 | @@ -23,9 +23,17 @@ namespace fsbridge {
  23 |  FILE *fopen(const fs::path& p, const char *mode)
  24 |  {
  25 |  #ifndef WIN32
  26 | -    return ::fopen(p.c_str(), mode);
  27 | +    FILE *fp = ::fopen(p.c_str(), mode);
  28 | +    if(fp && fcntl(::fileno(fp), F_SETFD, FD_CLOEXEC) == -1) {
    hebasto commented at 8:57 AM on October 28, 2026:

    Unfortunately, the consecutive fopen and fcntl(..., FD_CLOEXEC) calls are not atomic, which is a race in a multithreaded process.

    SirECMG commented at 4:45 PM on October 28, 2026:

    Yes, you are right, I will try to find a solution on how I can make it safe under multi threaded processes.

  9. ValeraFinebits commented at 2:07 PM on October 28, 2026: none

    I tested fsbridge::fopen at ca864d2fc1 with a standalone MSVC test using fs.cpp. The file was opened in append mode ("a"), and handle inheritance was enabled in CreateProcessW. After the parent closed the file, DeleteFileW failed with ERROR_SHARING_VIOLATION on the pre-PR code. With this change, deletion succeeded while the child was still alive.

    The compilation errors in fs_tests.cpp prevent test_bitcoin from building on Windows.

  10. in src/util/fs.cpp:40 in bb15d682c5 outdated 
      38 | +    switch(mode[0]) {
  39 | +        case 'r':
  40 | +            flags |= read_write ? O_RDWR : O_RDONLY;
  41 | +            break;
  42 | +        case 'w':
  43 | +            flags |= (read_write ? ORDWR :  O_WRONLY) | O_CREAT | O_TRUNC;
    ValeraFinebits commented at 6:33 PM on October 28, 2026: 
                flags |= (read_write ? O_RDWR :  O_WRONLY) | O_CREAT | O_TRUNC;
  11. in src/util/fs.cpp:56 in bb15d682c5 outdated 
      54 | +    const int fd{::open(p.c_str(), flags, 0666)};
  55 | +    if(fd == -1) return nullptr;
  56 | +
  57 | +    // Match fopen's initial position for append.
  58 | +    if(mode[0] == 'a') {
  59 | +        (void)::lseek(fd, 0, SEED_END);
    ValeraFinebits commented at 6:34 PM on October 28, 2026: 
            (void)::lseek(fd, 0, SEEK_END);
  12. SirECMG force-pushed on Oct 28, 2026
  13. ValeraFinebits commented at 6:37 PM on October 28, 2026: none

    typos

  14. SirECMG force-pushed on Oct 28, 2026
  15. SirECMG force-pushed on Oct 28, 2026
  16. SirECMG commented at 6:44 PM on October 28, 2026: none

    Thanks @ValeraFinebits. I fixed the typos. Let me know what you think.

  17. ValeraFinebits commented at 6:49 PM on October 28, 2026: none

    Thanks @ValeraFinebits. I fixed the typos. Let me know what you think.

    Could we also add <cstring> for std::strchr?

  18. SirECMG force-pushed on Oct 28, 2026
  19. SirECMG force-pushed on Oct 28, 2026
  20. SirECMG force-pushed on Oct 28, 2026
  21. SirECMG commented at 7:06 PM on October 28, 2026: none

    Added the missing include!

  22. winterrdog commented at 8:30 PM on October 28, 2026: contributor

    could you please squash your commits into two: one for the unit test and one for the fix ?

    see: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

  23. ValeraFinebits commented at 9:07 PM on October 28, 2026: none

    Tested on Windows at 0c9ab56: the new test passes with the fix and fails on the HANDLE_FLAG_INHERIT check when I revert the Windows change in fs.cpp. Restoring the fix makes it pass again.

  24. SirECMG force-pushed on Oct 28, 2026
  25. SirECMG commented at 9:30 PM on October 28, 2026: none

    could you please squash your commits into two: one for the unit test and one for the fix ?

    see: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

    I performed a squash to clean up the git history, as asked.

  26. util: Make fsbridge::fopen files non-inheritable 082b385445
  27. test: Verify fsbridge::fopen opens non-inheritable files 17ea8dd818
  28. SirECMG force-pushed on Oct 29, 2026
  29. winterrdog commented at 11:59 AM on October 30, 2026: contributor

    seems like this fixes #36668. you can add Fixes [#36668](/bitcoin-bitcoin/36668/) to the PR description to have it auto-close on merge

  30. DrahtBot removed the label CI failed on Oct 30, 2026
Contributors
SirECMGDrahtBothebastoValeraFinebitswinterrdog
Labels
Utils/log/libs
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-10-30 15:51 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me