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

luke-jr · 2021-07-08T06:55:18Z

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): boostorg/process#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).

tryphe · 2021-08-01T03:25:54Z

Concept ACK

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

luke-jr · 2021-08-01T05:11:11Z

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

Sjors · 2021-08-06T16:41:34Z

This is a bit beyond my understanding of processes. I tested (on macOS) that 3b6153b doesn't break HWI integration, so I have no objection.

src/util/system.h

laanwj · 2021-11-29T15:50:07Z

src/util/system.h

+    void on_exec_setup(Executor&exec) const
+    {
+        try {
+            for (auto it : fs::directory_iterator("/dev/fd")) {


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.

Github-Pull: bitcoin#22417 Rebased-From: 3b6153b

DrahtBot · 2022-03-02T18:16:11Z

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.

src/util/system.h

Sjors · 2022-05-12T08:35:23Z

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

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

Compiling with depends does work.

Update: compiling on master @ 4129134 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).

src/util/system.h

luke-jr · 2022-05-12T20:11:26Z

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)

DrahtBot · 2022-10-10T12:29:49Z

🐙 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".}

luke-jr · 2023-07-05T16:16:20Z

Rebased and squashed

fanquake · 2023-07-05T16:43:18Z

src/common/run_command.h

+#include <fcntl.h>
+#ifdef FD_CLOEXEC
+#include <unistd.h>
+#if defined(__GNUC__)


In-code warning suppression has recently been removed, so you can drop all of the #if defined(__GNUC__) blocks.

maflcko · 2024-04-05T07:34:10Z

Are you still working on this? There has been no reply to a review comment for about a year: #22417 (comment)

DrahtBot · 2024-04-10T11:13:38Z

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

fanquake · 2024-04-10T11:19:03Z

What is the status of this now that Boost Process is removed (given that this currently adds Boost Process code)?

fanquake · 2024-06-25T15:38:47Z

Closing, see above, and this is adding Boost Process code, which no-longer exists in this codebase.

Sjors · 2024-06-25T17:19:39Z

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

a0eed55 run_command: Enable close_fds option to avoid lingering fds (Luke Dashjr) c7c356a cpp-subprocess: Iterate through /proc/self/fd for close_fds option on Linux (Luke Dashjr) 4f5e04d Revert "remove unneeded close_fds option from cpp-subprocess" (Luke Dashjr) Pull request description: Picks up stale #30756, while addressing my fallback comment (#30756 (comment)). > 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](https://bugzilla.redhat.com/show_bug.cgi?id=1537564). > > (Equivalent to #22417 was for boost::process) ACKs for top commit: achow101: ACK a0eed55 hebasto: ACK a0eed55, tested on Ubuntu 25.04: vasild: ACK a0eed55 Tree-SHA512: 7dc1cb6cc1f45ff7c4f53512e400baad1a033b4ebf14ba6f6ffa38588314932d6d01ef67b197f081e8202bb802659ac6a87998277797721d6d7b20efde8e9a6b

fanquake added the Utils/log/libs label Jul 8, 2021

luke-jr force-pushed the bpchild_closefds branch 4 times, most recently from ded64d7 to b7c45c8 Compare July 8, 2021 07:41

fanquake requested a review from Sjors July 8, 2021 08:20

luke-jr force-pushed the bpchild_closefds branch from b7c45c8 to 3b6153b Compare July 8, 2021 18:29

luke-jr changed the title ~~util/system: Close non-std fds before execing slave processes~~ util/system: Close non-std fds when execing slave processes Jul 8, 2021

luke-jr mentioned this pull request Nov 2, 2021

util: Add -shutdownnotify option #23395

Merged

laanwj reviewed Nov 29, 2021

View reviewed changes

src/util/system.h Outdated Show resolved Hide resolved

laanwj reviewed Nov 29, 2021

View reviewed changes

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 14, 2021

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

712effe

Github-Pull: bitcoin#22417 Rebased-From: 3b6153b

DrahtBot mentioned this pull request Mar 2, 2022

refactor: Split ArgsManager out of util/system #24455

Closed

DrahtBot mentioned this pull request Apr 17, 2022

lint: Convert lint-includes.sh to Python #24895

Merged

DrahtBot added the Needs rebase label Apr 20, 2022

luke-jr force-pushed the bpchild_closefds branch 2 times, most recently from 7b0c71c to 4c19cea Compare May 12, 2022 07:00

Sjors reviewed May 12, 2022

View reviewed changes

src/util/system.h Outdated Show resolved Hide resolved

Sjors reviewed May 12, 2022

View reviewed changes

src/util/system.h Outdated Show resolved Hide resolved

luke-jr mentioned this pull request May 12, 2022

Re-enable external signer support for Windows #25111

Closed

luke-jr force-pushed the bpchild_closefds branch from 4c19cea to 2255d3b Compare August 31, 2022 00:25

DrahtBot removed the Needs rebase label Aug 31, 2022

DrahtBot mentioned this pull request Sep 28, 2022

kernel: move RunCommandParseJSON to its own file #26196

Merged

luke-jr marked this pull request as ready for review July 5, 2023 16:15

luke-jr force-pushed the bpchild_closefds branch from 2255d3b to 60bc072 Compare July 5, 2023 16:15

luke-jr requested review from Sjors, fanquake and laanwj July 5, 2023 16:16

fanquake removed Up for grabs Needs rebase labels Jul 5, 2023

fanquake reviewed Jul 5, 2023

View reviewed changes

DrahtBot added CI failed and removed CI failed labels Jul 5, 2023

achow101 requested a review from ryanofsky September 20, 2023 16:58

DrahtBot mentioned this pull request Dec 1, 2023

Replace Boost.Process with cpp-subprocess #28981

Merged

DrahtBot added CI failed and removed CI failed labels Dec 15, 2023

DrahtBot added the CI failed label Jan 14, 2024

DrahtBot mentioned this pull request Feb 28, 2024

[DO NOT MERGE] Replace Boost.Process with cpp-subprocess #29490

Closed

DrahtBot mentioned this pull request Mar 27, 2024

lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner #29744

Closed

DrahtBot added the Needs rebase label Apr 10, 2024

laanwj mentioned this pull request Apr 14, 2024

util: remove unused cpp-subprocess options #29865

Merged

fanquake closed this Jun 25, 2024

luke-jr mentioned this pull request Aug 29, 2024

run_command: Close non-std fds when execing slave processes #30756

Closed

laanwj mentioned this pull request Apr 24, 2025

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

Merged

bitcoin locked and limited conversation to collaborators Jun 25, 2025

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

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

Uh oh!

Conversation

luke-jr commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tryphe commented Aug 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Aug 1, 2021

Uh oh!

Sjors commented Aug 6, 2021

Uh oh!

Uh oh!

laanwj Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

Sjors commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

luke-jr commented May 12, 2022

Uh oh!

DrahtBot commented Oct 10, 2022

Uh oh!

luke-jr commented Jul 5, 2023

Uh oh!

fanquake Jul 5, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 5, 2024

Uh oh!

DrahtBot commented Apr 10, 2024

Uh oh!

fanquake commented Apr 10, 2024

Uh oh!

fanquake commented Jun 25, 2024

Uh oh!

Sjors commented Jun 25, 2024

Uh oh!

Uh oh!

luke-jr commented Jul 8, 2021 •

edited

Loading

tryphe commented Aug 1, 2021 •

edited

Loading

laanwj Nov 29, 2021 •

edited

Loading

DrahtBot commented Mar 2, 2022 •

edited

Loading

Sjors commented May 12, 2022 •

edited

Loading