run_command: Close non-std fds when execing slave processes #30756
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30756. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
3be95de to
7fff94a
Compare
6a3967f to
1e6b0f3
Compare
This reverts commit 79c3036.
1e6b0f3 to
28f6d4f
Compare
|
(Tangent: CI passed with 1e6b0f3ec3a878efb96019f40185809ed2b0699e, but it appears there's a CI bug making it fail with the equivalent 28f6d4fa7eb1b0da16ee940922c5c466da2ec0ec) |
|
Concept ACK. Closing unnecessary fds before starting external commands is good practice.
Pretty sure i brought up this exact concern prior to the cpp-subprocess cleanup, but was assured it wasn't removed. Apparently it was, one PR later.
i don't think this necessarily ports over to to non-Linux BSDs and such, but not sure, it's definitely a better way if it is available. |
28f6d4f to
ff198b5
Compare
|
Refactored to build a |
| if (parent_->close_fds_) { | ||
| try { | ||
| std::vector<int> fds_to_close; | ||
| for (const auto& it : fs::directory_iterator("/proc/self/fd")) { |
There was a problem hiding this comment.
This optimization is good to have, but imo we need the brute-force fallback in case this directory is not available. This can happen in the case of a chroot, filesystem namespacing, or on operating systems that don't have /proc/self.
This can be taken as-is from: https://github.com/arun11299/cpp-subprocess/blob/master/subprocess.hpp#L1768
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
Continued in #32343. |
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
Currently,
RunCommandParseJSONruns 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 forRunCommandParseJSON, and optimises it by iterating over/proc/self/fd/like most other libraries do these days (eg, glib) since iterating all possible fd numbers has been found to be problematic.(Equivalent to #22417 was for boost::process)