util: Revert “common: Close non-std fds before exec in RunCommandJSON” #33063

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2507-tempoary-revert changing 2 files +1 −59
  1. maflcko commented at 1:42 pm on July 25, 2025: member

    After a fork() in a multithreaded program, the child can safely call only async-signal-safe functions (see signal-safety(7)) 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 (https://github.com/bitcoin/bitcoin/pull/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 #32524 Fixes #33015 Fixes #32855

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

    • While forking, a debug mode mutex is held (by any other thread).
    • The forked child tries to use the stdard libary before execv and deadlocks.

    This may look like the following:

     0(gdb) thread apply all bt 
     1
     2Thread 1 (Thread 0xf58f4b40 (LWP 774911) "b-httpworker.2"):
     3[#0](/bitcoin-bitcoin/0/)  0xf7f4f589 in __kernel_vsyscall ()
     4[#1](/bitcoin-bitcoin/1/)  0xf79e467e in ?? () from /lib32/libc.so.6
     5[#2](/bitcoin-bitcoin/2/)  0xf79eb582 in pthread_mutex_lock () from /lib32/libc.so.6
     6[#3](/bitcoin-bitcoin/3/)  0xf7d93bf2 in ?? () from /lib32/libstdc++.so.6
     7[#4](/bitcoin-bitcoin/4/)  0xf7d93f36 in __gnu_debug::_Safe_iterator_base::_M_attach(__gnu_debug::_Safe_sequence_base*, bool) () from /lib32/libstdc++.so.6
     8[#5](/bitcoin-bitcoin/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
     9[#6](/bitcoin-bitcoin/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
    10[#7](/bitcoin-bitcoin/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
    11[#8](/bitcoin-bitcoin/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
    12[#9](/bitcoin-bitcoin/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
    13[#10](/bitcoin-bitcoin/10/) 0x57d83701 in subprocess::detail::Child::execute_child (this=0xf58f156c) at ./util/subprocess.h:1372
    14[#11](/bitcoin-bitcoin/11/) 0x57d80a7c in subprocess::Popen::execute_process (this=0xf58f1cd8) at ./util/subprocess.h:1231
    15[#12](/bitcoin-bitcoin/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
    16[#13](/bitcoin-bitcoin/13/) 0x57d6b597 in RunCommandParseJSON (str_command="fake.py enumerate", str_std_in="") at ./common/run_command.cpp:27
    17[#14](/bitcoin-bitcoin/14/) 0x57a90547 in ExternalSigner::Enumerate (command="fake.py", signers=std::__debug::vector of length 0, capacity 0, chain="regtest") at ./external_signer.cpp:28
    18[#15](/bitcoin-bitcoin/15/) 0x56defdab in enumeratesigners()::$_0::operator()(RPCHelpMan const&, JSONRPCRequest const&) const (this=0xf58f2ba0, self=..., request=...) at ./rpc/external_signer.cpp:51
    19...
    20(truncated, only one thread exists)
    
  2. Revert "Merge bitcoin/bitcoin#32343: common: Close non-std fds before exec in RunCommandJSON"
    This reverts commit 31d3eebfb92ae0521e18225d69be95e78fb02672, reversing
    changes made to 4b26ca0e2f1ec6b68861f1e8c4fd932f8fd8a271.
    faa1c3e80d
  3. DrahtBot added the label Utils/log/libs on Jul 25, 2025
  4. DrahtBot commented at 1:42 pm on July 25, 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/33063.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK darosior, fanquake
    Concept ACK Sjors, naiyoma

    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:

    • #32577 (subprocess: Let shell parse command on non-Windows systems 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.

  5. maflcko added this to the milestone 30.0 on Jul 25, 2025
  6. fanquake commented at 1:49 pm on July 25, 2025: member

    ACK - to stop the flow of CI failures.

    cc @hebasto @laanwj.

  7. in src/common/run_command.cpp:27 in faa1c3e80d
    23@@ -24,7 +24,7 @@ UniValue RunCommandParseJSON(const std::string& str_command, const std::string&
    24 
    25     if (str_command.empty()) return UniValue::VNULL;
    26 
    27-    auto c = sp::Popen(str_command, sp::input{sp::PIPE}, sp::output{sp::PIPE}, sp::error{sp::PIPE}, sp::close_fds{true});
    


    Sjors commented at 2:27 pm on July 25, 2025:
    Maybe only change this and don’t undo the already upstreamed change (yet)?

    Sjors commented at 2:34 pm on July 25, 2025:
    Oh never mind, we didn’t upstream this ourselves, it was already there and we copied it here.

    maflcko commented at 2:36 pm on July 25, 2025:

    Maybe only change this and don’t undo the already upstreamed change (yet)?

    I don’t understand what you are referring to. Can you provide a link?

  8. Sjors commented at 2:30 pm on July 25, 2025: member

    call only async-signal-safe functions

    Is this related? https://github.com/arun11299/cpp-subprocess/issues/53

  9. maflcko commented at 2:39 pm on July 25, 2025: member

    Is this related? arun11299/cpp-subprocess#53

    Yes. My recommendation would be to fix this upstream. I currently don’t plan to do it, so anyone else is free to pick it up.

  10. Sjors commented at 2:54 pm on July 25, 2025: member

    Concept ACK

    I agree it makes sense to fix upstream first.

    How do you verify a reverted merge commit? I guess this worked:

    0git checkout maflcko/2507-tempoary-revert
    1git cherry-pick 4f5e04da135080291853f71e6f81dd0302224c3a^..a0eed55398f882d9390e50582b10272d18f2b836
    2git diff maflcko/2507-tempoary-revert~1
    

    So do I understand correctly from the man page you linked to that close() and getpid and are already signal safe, and it’s just throw and sysconf that are the problem?

  11. maflcko commented at 2:59 pm on July 25, 2025: member

    it’s just throw and sysconf that are the problem?

    No, all of the C++ standard libary is problematic as well. (See the pull description and the bt)

    How do you verify a reverted merge commit? I guess this worked:

    I typed git revert -m1 31d3eebfb92ae0521e18225d69be95e78fb02672, but git is very flexible and you can type many different things to arrive at the same result.

  12. darosior commented at 3:01 pm on July 25, 2025: member
    Concept ACK
  13. darosior approved
  14. darosior commented at 3:07 pm on July 25, 2025: member
    ACK faa1c3e80d95552bdc2c0e717065ebf8d510138f
  15. DrahtBot requested review from fanquake on Jul 25, 2025
  16. DrahtBot requested review from Sjors on Jul 25, 2025
  17. naiyoma commented at 0:03 am on July 26, 2025: contributor

    Concept ACK

    From POSIX.1-2017 fork() documentation:

    ‘A process shall be created with a single thread. If a multi-threaded process calls fork(), the new process shall contain a replica of the calling thread and its entire address space, possibly including the states of mutexes and other resources. Consequently, to avoid errors, the child process may only execute async-signal-safe operations until such time as one of the exec functions is called.’

  18. fanquake commented at 9:17 am on July 26, 2025: member
    ACK faa1c3e80d95552bdc2c0e717065ebf8d510138f
  19. DrahtBot requested review from naiyoma on Jul 26, 2025
  20. fanquake merged this on Jul 26, 2025
  21. fanquake closed this on Jul 26, 2025

  22. maflcko deleted the branch on Jul 28, 2025

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-08-02 18:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me