common: Close non-std fds before exec in RunCommandJSON #32343

pull laanwj wants to merge 3 commits into bitcoin:master from laanwj:2025-04-closefds changing 2 files +59 −1
  1. laanwj commented at 7:01 pm on April 24, 2025: member

    Picks up stale #30756, while addressing my fallback comment (https://github.com/bitcoin/bitcoin/pull/30756#discussion_r2030844440).

    Currently, RunCommandParseJSON runs its target with whatever fds happen to be open inherited on POSIX platforms. I don’t think there’s any practical scenario where this is a problem right now, but there’s a lot of potential for weird problems (eg, if a process manages to outlive bitcoind - perhaps it’s hanging - the listening port(s) won’t get released and starting bitcoind again will fail). It’s also a potential security issue if a child process is intended to be sandboxed at some point. Not to mention plain ugly :)

    cpp-subprocess has a feature to address this called close_fds. Not sure why it was removed in #29961 rather than fixing this during the migration, but this PR restores it, enables it for RunCommandParseJSON, and optimises it by iterating over /proc/self/fd/ like most other libraries do these days ([eg, glib]> (https://gitlab.gnome.org/GNOME/glib/blob/487b1fd20c5e494366a82ddc0fa6b53b8bd779ad/glib/gspawn.c#L1094)) since iterating all possible fd numbers has been found to be problematic.

    (Equivalent to #22417 was for boost::process)

  2. laanwj added the label Utils/log/libs on Apr 24, 2025
  3. DrahtBot commented at 7:01 pm on April 24, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32343.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hebasto, vasild, achow101
    Concept ACK shahsb

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  4. laanwj force-pushed on Apr 24, 2025
  5. DrahtBot added the label CI failed on Apr 24, 2025
  6. DrahtBot commented at 7:07 pm on April 24, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/41111890297

    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.

  7. laanwj force-pushed on Apr 24, 2025
  8. laanwj commented at 7:54 pm on April 24, 2025: member

    This causes the signer test to fail (timeout) in the CentOS run, interesting, cannot reproduce it locally

    0[15:47:25.389] 
1wallet_signer.py --descriptors                                       | ✖ Failed  | 1205 s

    0[15:47:25.389] 
1                                   test_framework.authproxy.JSONRPCException: 'walletdisplayaddress' RPC took longer than 1200.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)

    i suspect this is actually a case where /proc/self/fd isn’t accessible so it gets to the fallback, which is so slow on this configuration that it times out. Ugh. Could go back to silently swallowing the exception, but also i’m not sure having this behavior conditionally is that useful at all (a security/robustness feature shouldn’t be so easy to accidentally disable).

    Apparently, doing this in a cross-platform is extremely difficult, mind that /proc/sys/fd is specific to Linux, and some platforms (like MacOS) don’t have a way at all: https://stackoverflow.com/a/56650580 . That leaves the brute-force option which can, as we see, be super slow.

  9. laanwj commented at 12:10 pm on April 25, 2025: member

    TIL Linux has close_range(first, last, flags) (borrowed from FreeBSD) which would be ideal for this, as it closes an entire range of file descriptors in one syscall. But unfortunately it was introduced in glibc 2.34, and we require only 2.31.

    Edit: i checked what actually leaks into a subprocess (using notify, not a signer, because it’s easier, but it’s going to be the same):

    0rm -f /tmp/open-files && ./launch-bitcoin-mainnet.sh -startupnotify="ls -al /proc/self/fd >> /tmp/open-files"

     0$ cat /tmp/open-files
 1total 0
 2dr-x------ 2 user user  0 Apr 25 15:15 .
 3dr-xr-xr-x 9 user user  0 Apr 25 15:15 ..
 4lrwx------ 1 user user 64 Apr 25 15:15 0 -> /dev/null  [EXPECTED]
 5l-wx------ 1 user user 64 Apr 25 15:15 1 -> /tmp/open-files [EXPECTED]
 6lrwx------ 1 user user 64 Apr 25 15:15 2 -> /dev/null  [EXPECTED]
 7lrwx------ 1 user user 64 Apr 25 15:15 25 -> socket:[8892399]
 8lrwx------ 1 user user 64 Apr 25 15:15 26 -> socket:[8892400]
 9lr-x------ 1 user user 64 Apr 25 15:15 27 -> /data/bitcoin/mempool.dat
10lrwx------ 1 user user 64 Apr 25 15:15 28 -> socket:[8899630]
11lr-x------ 1 user user 64 Apr 25 15:15 29 -> /data/bitcoin/onion_v3_private_key
12lr-x------ 1 user user 64 Apr 25 15:15 3 -> /proc/1261821/fd [EXPECTED]
13lrwx------ 1 user user 64 Apr 25 15:15 5 -> /data/bitcoin/.lock
14lrwx------ 1 user user 64 Apr 25 15:15 6 -> /data/bitcoin/blocks/.lock
15l-wx------ 1 user user 64 Apr 25 15:15 7 -> /data/bitcoin/debug.log

    Only the ones with [EXPECTED] should be there. Leaking the lock files is bad, because a child process that outlives bitcoind will hold on to them. Leaking a file descriptor to the onion key (but this is a race with -startupnotify, it isn’t persistently open) and debug log is potentially an info leak.

    The three sockets are from the bitcoin P2P code:

    0bitcoind  1261781                               user   25u     IPv4            8892399       0t0        TCP vm07.lan:18888 (LISTEN)
1bitcoind  1261781                               user   26u     IPv4            8892400       0t0        TCP localhost:8334 (LISTEN)
2bitcoind  1261781                               user   28u     IPv4            8899630       0t0        TCP localhost:58468->localhost:9051 (ESTABLISHED)
  10. vasild commented at 1:37 pm on April 25, 2025: contributor

    /proc/self/fd

    I would try something like this to check if /proc/self/fd is available:

     0--- i/ci/test/03_test_script.sh
 1+++ w/ci/test/03_test_script.sh
 2@@ -145,12 +145,17 @@ if [ "$RUN_UNIT_TESTS" = "true" ]; then
 3 fi
 4 
 5 if [ "$RUN_UNIT_TESTS_SEQUENTIAL" = "true" ]; then
 6   DIR_UNIT_TEST_DATA="${DIR_UNIT_TEST_DATA}" LD_LIBRARY_PATH="${DEPENDS_DIR}/${HOST}/lib" "${BASE_OUTDIR}"/bin/test_bi
 7 fi
 8
 9+ls -l /proc || :
10+ls -l /proc/self || :
11+ls -l /proc/self/fd || :
12+mount || :
13+
14 if [ "$RUN_FUNCTIONAL_TESTS" = "true" ]; then
15   # parses TEST_RUNNER_EXTRA as an array which allows for multiple arguments such as TEST_RUNNER_EXTRA='--exclude "rpc
16   eval "TEST_RUNNER_EXTRA=($TEST_RUNNER_EXTRA)"
17   LD_LIBRARY_PATH="${DEPENDS_DIR}/${HOST}/lib" test/functional/test_runner.py --ci "${MAKEJOBS}" --tmpdirprefix "${BAS
18 fi
  11. laanwj force-pushed on Apr 25, 2025
  12. laanwj force-pushed on Apr 25, 2025
  13. laanwj commented at 2:34 pm on April 25, 2025: member

    I would try something like this to check if /proc/self/fd is available:

    Thanks! Tried that, looks like it is:

     0[09:58:29.841] === BEGIN mount info ===
 1...
 2 + ls -al /proc/self/fd
 3[09:58:29.860] total 0
 4[09:58:29.860] dr-x------ 2 root root  4 Apr 25 09:58 .
 5[09:58:29.860] dr-xr-xr-x 9 root root  0 Apr 25 09:58 ..
 6[09:58:29.860] lr-x------ 1 root root 64 Apr 25 09:58 0 -> /dev/null
 7[09:58:29.860] l-wx------ 1 root root 64 Apr 25 09:58 1 -> pipe:[887125611]
 8[09:58:29.860] l-wx------ 1 root root 64 Apr 25 09:58 2 -> pipe:[887125612]
 9[09:58:29.860] lr-x------ 1 root root 64 Apr 25 09:58 3 -> /proc/69/fd
10...
11[09:58:29.872] === END mount info ===

    Not just that, but confirmed with some other additional logging in the unit tests that it’s actually closing fd’s through the directory iterator loop:

    0[10:02:47.377] 2025-04-25T14:02:47.353738Z [test] [common/run_command.cpp:33] [RunCommandParseJSON] Error output: pre fs::directory_iterator
1[10:02:47.377] closing: 3
2[10:02:47.377] closing: 4
3[10:02:47.377] closing: 5
4[10:02:47.377] closing: 6
5[10:02:47.377] closing: 7
6[10:02:47.377] post fs::directory_iterator

    So it doesn’t look like that is the problem.

  14. DrahtBot removed the label CI failed on Apr 25, 2025
  15. hebasto commented at 10:54 pm on April 26, 2025: member
    Concept ACK.
  16. maflcko commented at 8:53 am on April 27, 2025: member
    The error seems to be intermittent, because now it looks like the centos task passed. (For reference, the failure three days ago was https://github.com/bitcoin/bitcoin/runs/41112451543)
  17. laanwj force-pushed on Apr 27, 2025
  18. DrahtBot added the label CI failed on Apr 27, 2025
  19. maflcko commented at 1:56 pm on April 27, 2025: member
    Re-running the CentOS task shows that 1 in 5 runs fail
  20. laanwj force-pushed on Apr 28, 2025
  21. laanwj commented at 6:37 am on April 28, 2025: member

    i replaced the fallback by an explicit OSError (this should be propagated to the parent through the error pipe), maybe that will help diagnose the issue.

    (failure on MacOS is expected – there is no /proc)

    Edit: MacOS should pass now. The Linux code makes the hard assumption /proc/<pid>/fd is available, for other operating systems always use the fallback.

  22. DrahtBot removed the label CI failed on Apr 28, 2025
  23. laanwj force-pushed on Apr 28, 2025
  24. laanwj force-pushed on Apr 28, 2025
  25. maflcko commented at 11:24 am on April 28, 2025: member
    Scheduled 10 runs in https://cirrus-ci.com/task/5397894338969600, but it seems they are all passing. Huh
  26. laanwj commented at 11:27 am on April 28, 2025: member

    Scheduled 10 runs in https://cirrus-ci.com/task/5397894338969600, but it seems they are all passing. Huh

    It could be 4a6b99b3172fe007328bd260968acdb0dd78c569, though i have no idea why that would make a difference. In any case, nice that it passes now.

    Cleaning up and squashing the relevant commits.

  27. laanwj force-pushed on Apr 28, 2025
  28. in src/util/subprocess.h:1316 in 3acd934311 outdated 
    1311+        // For Linux, enumerate /proc/<pid>/fd.
1312+        try {
1313+          std::vector<int> fds_to_close;
1314+          for (const auto& it : fs::directory_iterator(strprintf("/proc/%d/fd", getpid()))) {
1315+            int64_t fd;
1316+            if (!ParseInt64(it.path().filename().native(), &fd)) continue;
    maflcko commented at 12:22 pm on April 28, 2025:
    style nit: fd’s probably can’t be negative nor include + as prefix? So ToIntegral<uint64_t> may be a better fit.
    laanwj commented at 4:57 pm on April 28, 2025:
    Thanks, yes, that would be betterr.
  29. laanwj force-pushed on Apr 28, 2025
  30. laanwj force-pushed on Apr 28, 2025
  31. laanwj commented at 5:33 pm on April 28, 2025: member

    3acd934311477ac88e1c3176aaeaec2b3ad35425 → bdf634918c32d3c0da2c2262252470d1755bb085

    • Use ToIntegral instead of ParseInt

    Edit: had to restart “previous releases, depends DEBUG” run, failure was some docker network problem not related to this PR

  32. shahsb commented at 5:15 am on April 30, 2025: none
    Concept ACK
  33. laanwj commented at 8:12 am on May 1, 2025: member

    i’m going to approach this differently, turning this PR draft for now.

    Going to remove https://github.com/bitcoin/bitcoin/pull/32343/commits/269c2a3dc5c3c85ce73f66c354895f768576ce51 here and try to upstream similar functionality optimizing close_fds (written more generally, not using bitcoin-core code) to cpp-subprocess. We can then use that code here. (no, see below)

  34. laanwj marked this as a draft on May 1, 2025
  35. laanwj marked this as a draft on May 1, 2025
  36. hebasto referenced this in commit 83acfacaf4 on May 1, 2025
  37. hebasto commented at 8:15 am on May 1, 2025: member
    FWIW, I’m running the current PR branch on some other OSes: https://github.com/hebasto/bitcoin-core-nightly/pull/51.
  38. laanwj commented at 8:24 am on May 1, 2025: member

    FWIW, I’m running the current PR branch on some other OSes: https://github.com/hebasto/bitcoin-core-nightly/pull/51.

    Testing is very welcome, the overall approach is going to stay the same.

  39. laanwj commented at 8:34 am on May 1, 2025: member

    (written more generally, not using bitcoin-core code)

    Okay, after a bit of consideration, maybe this was a bad idea: the directory iteration and string-parsing is going to be horrible.

    cpp-subprocess is C++11 only, while std::filesystem and std::from_chars are C++17. i really don’t want to go back to using locale-dependent number parsing functions (indirectly re-introducing them into bitcoin core).

    Edit: so, i promise to upstream it, but only after cpp-subprocess bumps the C++ version.

  40. hebasto commented at 9:15 am on May 1, 2025: member

    @laanwj

    Before undrafting this PR. could you please rebase it to include the merged #32071. Otherwise, NetBSD tests fail.

  41. Revert "remove unneeded close_fds option from cpp-subprocess" 
    This reverts commit 79c30363733503a1fb7d4c98aa0d56ced0be6e32.
    4f5e04da13
  42. cpp-subprocess: Iterate through /proc/self/fd for close_fds option on Linux 
    As an optimization, iterate through /proc/<pid>/fd on Linux.

Co-authored-by: laanwj <126646+laanwj@users.noreply.github.com>
    c7c356a448
  43. run_command: Enable close_fds option to avoid lingering fds a0eed55398
  44. laanwj force-pushed on May 1, 2025
  45. laanwj commented at 9:18 am on May 1, 2025: member

    Before undrafting this PR. could you please rebase it to include the merged #32071. Otherwise, NetBSD tests fail.

    Sure. Rebased to master.

  46. laanwj marked this as ready for review on May 1, 2025
  47. Sjors commented at 11:54 am on May 1, 2025: member
    @laanwj can you add some testing hints to the PR description? E.g. is there a way to deliberately trigger the potential bug that this fixes?
  48. laanwj commented at 12:09 pm on May 1, 2025: member

    [@laanwj can you add some testing hints to the PR description? E.g. is there a way to deliberately trigger the potential bug that this fixes?

    Sure: The simplest way to see if everything properly gets closed is to list /proc/self/fd at the beginning in the signer, as in #32343 (comment) .

    Alternatively, if you really want to create a bug, you could actively try to mess around with file descriptors that are already open at the start of the process. This could, say, mess with open P2P connections, or write to the debug log.

    e.g. call shutdown() on all file descriptors (not “close”, as that will just close the socket and do nothing interesting), this will probably cause the P2P binds to fail:

    0    int max_fd = sysconf(_SC_OPEN_MAX);
1    for (int i = 3; i < max_fd; i++) {
2        shutdown(i, SHUT_RDWR);
3    }

    Or: fork another time, then exit the parent process but keep the child in an infinite loop. The child will inherit the leaked file descriptors (in turn) and hold on to them. This might mess with locks, eg when you quit bitcoind then restart it, something will still be holding on to the lock files.

    i’ll look into adding a functional test for one of these (don’t think this needs to hold up this PR even longer though, it already has such a long history).

  49. hebasto approved
  50. hebasto commented at 2:08 pm on May 2, 2025: member

    ACK a0eed55398f882d9390e50582b10272d18f2b836, tested on Ubuntu 25.04:

    • On the master branch @ 68ac9f116c0228a277f18f60ba2278b56356e6ac:
     0$ cat /tmp/list-fd.sh 
 1#!/usr/bin/env bash
 2ls -al /proc/self/fd > /tmp/open-files
 3$ ./build/bin/bitcoind -regtest -daemon -signer=/tmp/list-fd.sh
 4$ ./build/bin/bitcoin-cli -regtest enumeratesigners  # Ignore errors
 5$ ./build/bin/bitcoin-cli -regtest stop
 6$ cat /tmp/open-files
 7total 0
 8dr-x------ 2 hebasto hebasto 10 May  2 15:05 .
 9dr-xr-xr-x 9 hebasto hebasto  0 May  2 15:05 ..
10lr-x------ 1 hebasto hebasto 64 May  2 15:05 0 -> pipe:[231179]
11l-wx------ 1 hebasto hebasto 64 May  2 15:05 1 -> /tmp/open-files
12l-wx------ 1 hebasto hebasto 64 May  2 15:05 2 -> pipe:[231177]
13lrwx------ 1 hebasto hebasto 64 May  2 15:05 24 -> socket:[235679]
14lrwx------ 1 hebasto hebasto 64 May  2 15:05 25 -> socket:[235680]
15lrwx------ 1 hebasto hebasto 64 May  2 15:05 26 -> socket:[235681]
16lr-x------ 1 hebasto hebasto 64 May  2 15:05 3 -> /proc/61760/fd
17lrwx------ 1 hebasto hebasto 64 May  2 15:05 5 -> /home/hebasto/.bitcoin/regtest/.lock
18lrwx------ 1 hebasto hebasto 64 May  2 15:05 6 -> /home/hebasto/.bitcoin/regtest/blocks/.lock
19l-wx------ 1 hebasto hebasto 64 May  2 15:05 7 -> /home/hebasto/.bitcoin/regtest/debug.log
    • this PR:
     0$ ./build/bin/bitcoind -regtest -daemon -signer=/tmp/list-fd.sh
 1$ ./build/bin/bitcoin-cli -regtest enumeratesigners  # Ignore errors
 2$ ./build/bin/bitcoin-cli -regtest stop
 3$ cat /tmp/open-files
 4total 0
 5dr-x------ 2 hebasto hebasto  4 May  2 15:08 .
 6dr-xr-xr-x 9 hebasto hebasto  0 May  2 15:08 ..
 7lr-x------ 1 hebasto hebasto 64 May  2 15:08 0 -> pipe:[225165]
 8l-wx------ 1 hebasto hebasto 64 May  2 15:08 1 -> /tmp/open-files
 9l-wx------ 1 hebasto hebasto 64 May  2 15:08 2 -> pipe:[225163]
10lr-x------ 1 hebasto hebasto 64 May  2 15:08 3 -> /proc/62155/fd
  51. DrahtBot requested review from shahsb on May 2, 2025
  52. laanwj commented at 2:51 pm on May 5, 2025: member

    Some more “buggy” examples

    • Find the file descriptor for the debug log, then log some bootleg messages (needs Linux due to /proc/.../fd usage)
     0#!/usr/bin/env python3
 1# evil-signer1.py
 2import os, socket, sys
 3
 4fd_root = f'/proc/{os.getpid()}/fd'
 5open_files = []
 6for fname in os.listdir(fd_root):
 7    try:
 8        fd = int(fname)
 9        if fd < 3:
10            continue
11        link = os.readlink(os.path.join(fd_root, fname))
12        open_files.append((fd, link))
13    except OSError:
14        pass
15
16# find and open debug log
17debuglog = None
18for (fd, link) in open_files:
19    if link.endswith('/debug.log'):
20        debuglog = os.fdopen(fd, 'a')
21
22if debuglog is None:
23    sys.exit(1)
24
25debuglog.write(f'[-----------START----------]\n')
26for (fd, link) in open_files:
27    debuglog.write(f'file: {fd} {link}\n')
28debuglog.write(f'[------------END-----------]\n')
29debuglog.flush()

     0$ bin/bitcoind -regtest -daemonwait -signer=$HOME/2025-05-closefd-tests/evil-signer1.py
 1$ bin/bitcoin-cli -regtest enumeratesigners
 2(ignore error)
 3$ tail -f $HOME/.bitcoin/regtest/debug.log
 4[-----------START----------]
 5file: 5 /data/bitcoin/regtest/.lock
 6file: 7 /data/bitcoin/regtest/blocks/.lock
 7file: 8 /data/bitcoin/regtest/debug.log
 8file: 25 socket:[26621166]
 9file: 26 socket:[26621167]
10file: 27 socket:[26621168]
11file: 28 socket:[26410879]
12[------------END-----------]
    • This shuts down all leaked sockets, including listening ones (works for any UNIX-ish)
     0#!/usr/bin/env python3
 1# evil-signer2.py
 2import os, socket, sys
 3
 4MAX_FD = os.sysconf("SC_OPEN_MAX")
 5for fd in range(3, MAX_FD):
 6    try:
 7        sock = socket.fromfd(fd, socket.AF_INET, socket.SOCK_STREAM)
 8        sock.shutdown(socket.SHUT_RDWR)
 9    except OSError:
10        pass

    Example usage (like above):

    0$ bin/bitcoind -regtest -daemonwait -signer=$HOME/2025-05-closefd-tests/evil-signer2.py
1$ nc -w0 -v 127.0.0.1 18444
2Connection to 127.0.0.1 18444 port [tcp/*] succeeded!
3$ bin/bitcoin-cli -regtest enumeratesigners
4(ignore error)
5$ nc -v 127.0.0.1 18444 -w0
6nc: connect to 127.0.0.1 port 18444 (tcp) failed: Connection refused
    • Keep sockets and other things occupied for a minute
     0#!/usr/bin/env python3
 1# evil-signer3.py
 2import os, sys, time
 3if os.fork() > 0: # exit parent
 4    sys.exit(1)
 5# detach from terminal
 6os.close(0)
 7os.close(1)
 8os.close(2)
 9# keep sockets and locks occupied for a minute
10time.sleep(60)

    0$ bin/bitcoind -regtest -daemonwait -signer=$HOME/2025-05-closefd-tests/evil-signer3.py
1$ bin/bitcoin-cli -regtest enumeratesigners
2(ignore error)
3$ bin/bitcoin-cli -regtest stop
4$ bin/bitcoind -regtest -daemonwait -signer=$HOME/2025-05-closefd-tests/evil-signer3.py
5Bitcoin Core starting
6Error during initialization - check debug.log for details

    (sorry… i’ll stop here, i enjoy this kind of thing too much)

  53. hebasto commented at 12:41 pm on May 9, 2025: member

    @laanwj

    Are you going to upstream the second commit?

  54. laanwj commented at 12:46 pm on May 9, 2025: member

    Are you going to upstream the second commit?

    Yes, but only after cpp-subprocess starts using c++17: #32343 (comment)

    Edit: whatever comes first though; being able to use close_range, introduced in n glibc 2.34 would be superior over the proc iteration approach: #32343 (comment) . Could do it in two syscalls: close_range(3, err_wr_pipe_ - 1); close_range(err_wr_pipe_ + 1, MAXINT). But alas.

  55. in src/util/subprocess.h:1323 in a0eed55398 
    1318+            if (*fd == static_cast<uint64_t>(err_wr_pipe_)) continue;
1319+            fds_to_close.push_back(*fd);
1320+          }
1321+          for (const int fd : fds_to_close) {
1322+            close(fd);
1323+          }
    vasild commented at 1:30 pm on May 12, 2025:
    Can close() be called right away at line 1319 and avoid the fds_to_close variable and the second for loop?
    laanwj commented at 2:06 pm on May 12, 2025:
    Not sure if changing the contents during iteration could mess with the directory iteration. This feels safer.
  56. in src/util/subprocess.h:1335 in a0eed55398 
    1330+        int max_fd = sysconf(_SC_OPEN_MAX);
1331+        if (max_fd == -1) throw OSError("sysconf failed", errno);
1332+
1333+        for (int i = 3; i < max_fd; i++) {
1334+          if (i == err_wr_pipe_) continue;
1335+          close(i);
    vasild commented at 1:43 pm on May 12, 2025:

    Nothing prevents another thread from opening a new file after all these close() calls have completed and before the execvp() call below. So I guess this is best effort attempt?

    A sure way would be to use O_CLOEXEC when opening a file: https://www.man7.org/linux/man-pages/man2/open.2.html.

    laanwj commented at 2:05 pm on May 12, 2025:

    There are no threads at this point, mind this is after the fork(), in the child process.

    A sure way would be to use O_CLOEXEC when opening a file: https://www.man7.org/linux/man-pages/man2/open.2.html.

    Sure, that’s better (and done for quite a lot of things, seeing that not everything leaks. It’s also why the error pipe doesn’t leak after exec) but it’s impossible to enforce this on all libraries too, so easy to miss something.

  57. vasild approved
  58. vasild commented at 1:46 pm on May 12, 2025: contributor
    ACK a0eed55398f882d9390e50582b10272d18f2b836
  59. achow101 commented at 11:07 pm on May 14, 2025: member

    ACK a0eed55398f882d9390e50582b10272d18f2b836

    Tested manually with a modified mock signer. Prior to this change, several fds exist referring to various files and sockets opened by the node. With this PR, fds 0, 1, and 2 remain open as pipes, and the rest are for files opened by the mock signer itself.

  60. achow101 merged this on May 14, 2025
  61. achow101 closed this on May 14, 2025


laanwj DrahtBot vasild hebasto maflcko shahsb Sjors achow101


shahsb

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: 2025-05-20 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me