Merge bitcoin#33063: util: Revert "common: Close non-std fds before exec in RunCommandJSON"

faa1c3e Revert "Merge bitcoin#32343: common: Close non-std fds before exec in RunCommandJSON" (MarcoFalke)

Pull request description:

  After a fork() in a multithreaded program, the child can safely
  call only async-signal-safe functions (see [signal-safety(7)](https://www.man7.org/linux/man-pages/man7/signal-safety.7.html))
  until such time as it calls execv.

  The standard library (`std` namespace) is not async-signal-safe. Also, `throw`, isn't.

  There was an alternative implementation using `readdir` (bitcoin#32529), but that isn't async-signal-safe either, and that implementation was still using `throw`.

  So temporarily revert this feature.

  A follow-up in the future can add it back, using only async-signal-safe functions, or by using a different approach.

  Fixes bitcoin#32524
  Fixes bitcoin#33015
  Fixes bitcoin#32855

  For reference, a failure can manifest in the GCC debug mode:

  * While `fork`ing, a debug mode mutex is held (by any other thread).
  * The `fork`ed child tries to use the stdard libary before `execv` and deadlocks.

  This may look like the following:

  ```
  (gdb) thread apply all bt

  Thread 1 (Thread 0xf58f4b40 (LWP 774911) "b-httpworker.2"):
  #0  0xf7f4f589 in __kernel_vsyscall ()
  bitcoin#1  0xf79e467e in ?? () from /lib32/libc.so.6
  #2  0xf79eb582 in pthread_mutex_lock () from /lib32/libc.so.6
  #3  0xf7d93bf2 in ?? () from /lib32/libstdc++.so.6
  #4  0xf7d93f36 in __gnu_debug::_Safe_iterator_base::_M_attach(__gnu_debug::_Safe_sequence_base*, bool) () from /lib32/libstdc++.so.6
  #5  0x5668810a in __gnu_debug::_Safe_iterator_base::_Safe_iterator_base (this=0xf58f13ac, __seq=0xf58f13f8, __constant=false) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_base.h:91
  #6  0x56ddfb50 in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::forward_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:162
  #7  0x56ddfacb in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::bidirectional_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:539
  #8  0x56ddfa5b in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::random_access_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:687
  #9  0x56ddd3f6 in std::__debug::vector<int, std::allocator<int> >::begin (this=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/vector:300
  #10 0x57d83701 in subprocess::detail::Child::execute_child (this=0xf58f156c) at ./util/subprocess.h:1372
  #11 0x57d80a7c in subprocess::Popen::execute_process (this=0xf58f1cd8) at ./util/subprocess.h:1231
  #12 0x57d6d2b4 in subprocess::Popen::Popen<subprocess::input, subprocess::output, subprocess::error, subprocess::close_fds> (this=0xf58f1cd8, cmd_args="fake.py enumerate", args=..., args=..., args=..., args=...) at ./util/subprocess.h:964
  #13 0x57d6b597 in RunCommandParseJSON (str_command="fake.py enumerate", str_std_in="") at ./common/run_command.cpp:27
  #14 0x57a90547 in ExternalSigner::Enumerate (command="fake.py", signers=std::__debug::vector of length 0, capacity 0, chain="regtest") at ./external_signer.cpp:28
  #15 0x56defdab in enumeratesigners()::$_0::operator()(RPCHelpMan const&, JSONRPCRequest const&) const (this=0xf58f2ba0, self=..., request=...) at ./rpc/external_signer.cpp:51
  ...
  (truncated, only one thread exists)
  ```

ACKs for top commit:
  fanquake:
    ACK faa1c3e
  darosior:
    ACK faa1c3e

Tree-SHA512: 602da5f2eba08d7fe01ba19baf411e287ae27fe2d4b82f41734e05b7b1d938ce94cc0041e86ba677284fa92838e96ebee687023ff28047e2b036fd9a53567e0a

Author: fanquake

src/common/run_command.cpp

@@ -24,7 +24,7 @@ UniValue RunCommandParseJSON(const std::string& str_command, const std::string&
 
     if (str_command.empty()) return UniValue::VNULL;
 
-    auto c = sp::Popen(str_command, sp::input{sp::PIPE}, sp::output{sp::PIPE}, sp::error{sp::PIPE}, sp::close_fds{true});
+    auto c = sp::Popen(str_command, sp::input{sp::PIPE}, sp::output{sp::PIPE}, sp::error{sp::PIPE});
     if (!str_std_in.empty()) {
         c.send(str_std_in);
     }

src/util/subprocess.h

@@ -36,8 +36,6 @@ Documentation for C++ subprocessing library.
 #ifndef BITCOIN_UTIL_SUBPROCESS_H
 #define BITCOIN_UTIL_SUBPROCESS_H
 
-#include <util/fs.h>
-#include <util/strencodings.h>
 #include <util/syserror.h>
 
 #include <algorithm>
@@ -538,20 +536,6 @@ namespace util
  * -------------------------------
  */
 
-/*!
- * Option to close all file descriptors
- * when the child process is spawned.
- * The close fd list does not include
- * input/output/error if they are explicitly
- * set as part of the Popen arguments.
- *
- * Default value is false.
- */
-struct close_fds {
-  explicit close_fds(bool c): close_all(c) {}
-  bool close_all = false;
-};
-
 /*!
  * Base class for all arguments involving string value.
  */
@@ -749,7 +733,6 @@ struct ArgumentDeducer
   void set_option(input&& inp);
   void set_option(output&& out);
   void set_option(error&& err);
-  void set_option(close_fds&& cfds);
 
 private:
   Popen* popen_ = nullptr;
@@ -1043,8 +1026,6 @@ class Popen
   std::future<void> cleanup_future_;
 #endif
 
-  bool close_fds_ = false;
-
   std::string exe_name_;
 
   // Command in string format
@@ -1293,10 +1274,6 @@ namespace detail {
     if (err.rd_ch_ != -1) popen_->stream_.err_read_ = err.rd_ch_;
   }
 
-  inline void ArgumentDeducer::set_option(close_fds&& cfds) {
-    popen_->close_fds_ = cfds.close_all;
-  }
-
 
   inline void Child::execute_child() {
 #ifndef __USING_WINDOWS__
@@ -1343,41 +1320,6 @@ namespace detail {
       if (stream.err_write_ != -1 && stream.err_write_ > 2)
         subprocess_close(stream.err_write_);
 
-      // Close all the inherited fd's except the error write pipe
-      if (parent_->close_fds_) {
-        // If possible, try to get the list of open file descriptors from the
-        // operating system. This is more efficient, but not guaranteed to be
-        // available.
-#ifdef __linux__
-        // For Linux, enumerate /proc/<pid>/fd.
-        try {
-          std::vector<int> fds_to_close;
-          for (const auto& it : fs::directory_iterator(strprintf("/proc/%d/fd", getpid()))) {
-            auto fd{ToIntegral<uint64_t>(it.path().filename().native())};
-            if (!fd || *fd > std::numeric_limits<int>::max()) continue;
-            if (*fd <= 2) continue;  // leave std{in,out,err} alone
-            if (*fd == static_cast<uint64_t>(err_wr_pipe_)) continue;
-            fds_to_close.push_back(*fd);
-          }
-          for (const int fd : fds_to_close) {
-            close(fd);
-          }
-        } catch (const fs::filesystem_error &e) {
-          throw OSError("/proc/<pid>/fd iteration failed", e.code().value());
-        }
-#else
-        // On other operating systems, iterate over all file descriptor slots
-        // and try to close them all.
-        int max_fd = sysconf(_SC_OPEN_MAX);
-        if (max_fd == -1) throw OSError("sysconf failed", errno);
-
-        for (int i = 3; i < max_fd; i++) {
-          if (i == err_wr_pipe_) continue;
-          close(i);
-        }
-#endif
-      }
-
       // Replace the current image with the executable
       sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data());