util/system: Close non-std fds when execing slave processes #22417

1. 
   luke-jr commented at 6:55 am on July 8, 2021: member

   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 :)

   Note that boost::process has a boost::process::limit_handles extension for this, but it requires a newer Boost and is even in the latest version still somewhat broken on Windows (where it probably doesn’t matter since handle inheritance is off by default - but will execute undefined behaviour dereferencing/writing of an end-iterator): https://github.com/boostorg/process/issues/200

   Instead of closing directly, we have to set the FD_CLOEXEC flag and let exec close them; otherwise, boost::process’s internal pipe would be closed and we get the wrong exception thrown (not actually a problem outside of our tests, but might as well do this cleanly).

2. fanquake added the label Utils/log/libs on Jul 8, 2021
3. luke-jr force-pushed on Jul 8, 2021
4. luke-jr force-pushed on Jul 8, 2021
5. luke-jr force-pushed on Jul 8, 2021
6. luke-jr force-pushed on Jul 8, 2021
7. fanquake requested review from Sjors on Jul 8, 2021
8. luke-jr force-pushed on Jul 8, 2021
9. luke-jr renamed this:
   util/system: Close non-std fds before execing slave processes
   util/system: Close non-std fds when execing slave processes
   on Jul 8, 2021
10. 
    tryphe commented at 3:25 am on August 1, 2021: contributor

    Concept ACK

    Is it possible to easily test this functionality? I would like to give it a go.

11. 
    luke-jr commented at 5:11 am on August 1, 2021: member

    Should be possible with a long-running process, then looking at /proc/*pid*/fd or such.

    If you want to create a problem, write a program to write to random fd numbers (likely will corrupt things, don’t try on an important node). :p

12. 
    Sjors commented at 4:41 pm on August 6, 2021: member

    This is a bit beyond my understanding of processes. I tested (on macOS) that 3b6153ba336b929b06930d81930077f6bb519732 doesn’t break HWI integration, so I have no objection.
13. in src/util/system.h:145 in 3b6153ba33 outdated

    124+        try {
    125+            for (auto it : fs::directory_iterator("/dev/fd")) {
    126+                int64_t fd;
    127+                if (!ParseInt64(it.path().filename().native(), &fd)) continue;
    128+                if (fd <= 2) continue;  // leave std{in,out,err} alone
    129+                ::fcntl(fd, F_SETFD, ::fcntl(fd, F_GETFD) | FD_CLOEXEC);
    

    ---

    ---

    

    laanwj commented at 3:48 pm on November 29, 2021:

    As I understand this code is executed in the child process. What’s the advantage of using fcntl(…FD_CLOEXEC instead of straight-up closing the fd here?

    ---

    

    luke-jr commented at 5:06 pm on November 29, 2021:

    Instead of closing directly, we have to set the FD_CLOEXEC flag and let exec close them; otherwise, boost::process’s internal pipe would be closed and we get the wrong exception thrown (not actually a problem outside of our tests, but might as well do this cleanly).

14. in src/util/system.h:141 in 3b6153ba33 outdated

    120+{
    121+    template<typename Executor>
    122+    void on_exec_setup(Executor&exec) const
    123+    {
    124+        try {
    125+            for (auto it : fs::directory_iterator("/dev/fd")) {
    

    ---

    ---

    

    laanwj commented at 3:50 pm on November 29, 2021:

    TIL /dev/fd is a general UNIX thing and not limited to Linux. It doesn’t seem like a very nice API to iterate open file descriptors but seems portable enough.
15. luke-jr referenced this in commit 712effe57b on Dec 14, 2021
16. 
    DrahtBot commented at 6:16 pm on March 2, 2022: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type	Reviewers
    Concept ACK	tryphe

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29744 (lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner by davidgumberg)
    • #28981 (Replace Boost.Process with cpp-subprocess 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.

17. DrahtBot added the label Needs rebase on Apr 20, 2022
18. luke-jr force-pushed on May 12, 2022
19. luke-jr force-pushed on May 12, 2022
20. in src/util/system.h:42 in 4c19cea484 outdated

    34@@ -34,6 +35,29 @@
    35 #include <utility>
    36 #include <vector>
    37 
    38+#if defined(ENABLE_EXTERNAL_SIGNER) && defined(BOOST_POSIX_API)
    39+#include <fcntl.h>
    40+#ifdef FD_CLOEXEC
    41+#include <unistd.h>
    42+#if defined(WIN32) && !defined(__kernel_entry)
    

    ---

    ---

    

    Sjors commented at 8:28 am on May 12, 2022:

    Would it make sense to move all Windows related stuff from this PR to #25111?

    ---

    

    luke-jr commented at 8:32 am on May 12, 2022:

    This wouldn’t exist in #25111

    ---

    

    Sjors commented at 8:59 am on May 12, 2022:

    Isn’t defined(WIN32) always false on the systems that we support boost::process for? (since #24065)
21. 
    Sjors commented at 8:35 am on May 12, 2022: member

    Compiling 4c19cea484b1e88618864257253c2caf7d102dd6 on macOS 12.3.1 (Boost 1.78 via Homebrew) fails with:

    0wallet/test/init_test_fixture.cpp:35:52: error: no member named 'BOOST_FILESYSTEM_C_STR' in 'boost::filesystem::path'
    1    std::ofstream f(m_walletdir_path_cases["file"].BOOST_FILESYSTEM_C_STR);
    2                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
    31 error generated.
    4make[2]: *** [wallet/test/test_test_bitcoin-init_test_fixture.o] Error 1
    

    Compiling with depends does work.

    Update: compiling on master @ 4129134e844f78a89f8515cf30dad4b6074703c7 also fails, so this is unrelated (and already fixed, see #22482). Maybe rebase?

    For some reason CI has not run yet on this PR (probably because it builds on top of commits that use a previous CI incarnation).

22. in src/util/system.h:46 in 4c19cea484 outdated

    41+#include <unistd.h>
    42+#if defined(WIN32) && !defined(__kernel_entry)
    43+// A workaround for boost 1.71 incompatibility with mingw-w64 compiler.
    44+// For details see https://github.com/bitcoin/bitcoin/pull/22348.
    45+#define __kernel_entry
    46+#endif
    

    ---

    ---

    

    Sjors commented at 9:17 am on May 12, 2022:

    nit (if you keep it): // WIN32 && !kernel_entry
23. 
    luke-jr commented at 8:11 pm on May 12, 2022: member

    Testing and CI should always be merging the PR into master. The PR itself should ideally in the case of bugfixes like this, be based on the oldest affected commit (that can still cleanly merge, in Core’s case)
24. luke-jr force-pushed on Aug 31, 2022
25. DrahtBot removed the label Needs rebase on Aug 31, 2022
26. 
    DrahtBot commented at 12:29 pm on October 10, 2022: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

27. DrahtBot added the label Needs rebase on Oct 10, 2022
28. 
    fanquake commented at 11:27 am on December 5, 2022: member

    @luke-jr are you planning on rebasing here?

    Maybe @hebasto or @Sjors can re-review after that?

    Looks like the second commit (2255d3bc827fac4a0a343d25e3e1af1a53d5dbb2) is just dropping code that you’re introducing in the first commit?

29. fanquake marked this as a draft on Feb 6, 2023
30. 
    fanquake commented at 2:59 pm on February 6, 2023: member

    Moving to draft for now. No response to the recent comments or query about the redundant second commit. Has needed rebase for 4 months.
31. maflcko added the label Up for grabs on Feb 6, 2023
32. fanquake closed this on Mar 31, 2023

    ---

33. achow101 reopened this on Jul 5, 2023

    ---

34. util/system: Close non-std fds when execing slave processes 60bc072bc3
35. luke-jr marked this as ready for review on Jul 5, 2023
36. luke-jr force-pushed on Jul 5, 2023
37. 
    luke-jr commented at 4:16 pm on July 5, 2023: member

    Rebased and squashed
38. luke-jr requested review from Sjors on Jul 5, 2023
39. luke-jr requested review from fanquake on Jul 5, 2023
40. luke-jr requested review from laanwj on Jul 5, 2023
41. fanquake removed the label Up for grabs on Jul 5, 2023
42. fanquake removed the label Needs rebase on Jul 5, 2023
43. in src/common/run_command.h:21 in 60bc072bc3

    16 
    17+#if defined(ENABLE_EXTERNAL_SIGNER) && defined(BOOST_POSIX_API)
    18+#include <fcntl.h>
    19+#ifdef FD_CLOEXEC
    20+#include <unistd.h>
    21+#if defined(__GNUC__)
    

    ---

    ---

    

    fanquake commented at 4:43 pm on July 5, 2023:

    In-code warning suppression has recently been removed, so you can drop all of the #if defined(__GNUC__) blocks.
44. DrahtBot added the label CI failed on Jul 5, 2023
45. DrahtBot removed the label CI failed on Jul 6, 2023
46. achow101 requested review from ryanofsky on Sep 20, 2023
47. DrahtBot added the label CI failed on Dec 15, 2023
48. DrahtBot removed the label CI failed on Dec 19, 2023
49. DrahtBot added the label CI failed on Jan 14, 2024
50. 
    maflcko commented at 7:34 am on April 5, 2024: member

    Are you still working on this? There has been no reply to a review comment for about a year: #22417 (review)
51. 
    DrahtBot commented at 11:13 am on April 10, 2024: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

52. DrahtBot added the label Needs rebase on Apr 10, 2024
53. 
    fanquake commented at 11:19 am on April 10, 2024: member

    What is the status of this now that Boost Process is removed (given that this currently adds Boost Process code)?
54. 
    fanquake commented at 3:38 pm on June 25, 2024: member

    Closing, see above, and this is adding Boost Process code, which no-longer exists in this codebase.
55. fanquake closed this on Jun 25, 2024

    ---

56. 
    Sjors commented at 5:19 pm on June 25, 2024: member

    What is the status of this now that Boost Process is removed

    For historical reference: this patch is no longer relevant, since #28981 replaced boost-process with cpp-subprocess. @luke-jr does cpp-subprocess do anything similar that needs addressing? https://github.com/bitcoin/bitcoin/blob/master/src/util/subprocess.h

Contributors
luke-jr tryphe Sjors laanwj DrahtBot fanquake maflcko

Review Requested
Sjors laanwj ryanofsky

Labels
Utils/log/libs Needs rebase CI failed