Fix missing error check in set_clo_on_exec for FD_CLOEXEC handling #32342

1. 
   tomasandroil commented at 6:00 pm on April 24, 2025: none

   Changes:

   • Replaces unchecked fcntl(fd, F_SETFD, flags); with a version that checks the return value.
   • Throws OSError on failure with an appropriate error message.

   Motivation:

   Proper error handling is critical in system-level code to avoid silent failures, especially when setting file descriptor flags like FD_CLOEXEC. This fix ensures robustness and helps catch configuration issues early during process setup.

   Test coverage

   No functional change beyond improved error propagation, but this section of code is indirectly exercised by any subprocess-related integration or functional tests. Consider adding a unit test for set_clo_on_exec in the future for completeness.

2. 
   DrahtBot commented at 6:00 pm on April 24, 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/32342.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	shahsb

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

3. in src/util/subprocess.h:336 in 213d53b74f outdated

   335@@ -336,8 +336,9 @@ namespace util
   336     int flags = fcntl(fd, F_GETFD, 0);
   

   ---

   ---

   

   laanwj commented at 6:09 pm on April 24, 2025:

   This fcntl can return -1 on error too.
4. laanwj added the label Utils/log/libs on Apr 24, 2025
5. 
   tomasandroil commented at 6:45 pm on April 24, 2025: none

   @laanwj Thanks for the review! I’ve updated set_clo_on_exec to check the return value of fcntl(fd, F_GETFD, 0) and throw OSError on failure, similar to the existing check for F_SETFD
6. 
   laanwj commented at 7:22 pm on April 24, 2025: member

   LGTM now, raising OSError appears to be how other errors are handled in subprocess as well, so it’s at least consistent. Needs to be tested, though, i don’t really know the effect of this up the callchain. It doesn’t look like we actually catch OSError in RunCommandParseJSON.

   Please squash your commits and use a more informative commit message than “Update subprocess.h” .

7. fix(subprocess): check and handle fcntl(F_GETFD) failure

   Add missing error check for fcntl(fd, F_GETFD, 0) in set_clo_on_exec.
   Raise OSError on failure to align with existing FD_SETFD behavior.
   This improves robustness in subprocess setup and error visibility.

   b0415ab50e
8. tomasandroil force-pushed on Apr 24, 2025
9. 
   tomasandroil commented at 7:48 pm on April 24, 2025: none

   @laanwj I’ve squashed the commits and updated the message as requested
10. 
    shahsb commented at 5:50 am on April 25, 2025: none

    Concept ACK
11. 
    shahsb commented at 5:51 am on April 25, 2025: none

    ACK https://github.com/bitcoin/bitcoin/pull/32342/commits/b0415ab50e6d58375063e4616a0aad7788a6128e
12. 
    fanquake commented at 8:45 am on April 25, 2025: member

    This should probably be going upstream (https://github.com/arun11299/cpp-subprocess). cc @hebasto.
13. 
    laanwj commented at 11:35 am on April 25, 2025: member

    This should probably be going upstream (https://github.com/arun11299/cpp-subprocess). cc @hebasto.

    Same for the subprocess commit of #32343. Optimizing close-all-file-descriptors is out of scope here but it’s apparently needed to even pass the CI.

14. 
    hebasto commented at 7:03 pm on April 25, 2025: member

    Changes look good.

    This should probably be going upstream (https://github.com/arun11299/cpp-subprocess). cc @hebasto. @tomasandroil

    Yes, please consider upstreaming the changes.

15. 
    tomasandroil commented at 9:39 am on April 28, 2025: none

    @hebasto I have upstreamed the changes as requested: arun11299/cpp-subprocess#117.
16. 
    hebasto commented at 4:16 pm on April 28, 2025: member

    @tomasandroil

    I have upstreamed the changes as requested: arun11299/cpp-subprocess#117.

    Thank you!

    I’ve backported the merged upstream PR in #32358.

    So this one can be closed, I think.

17. 
    tomasandroil commented at 5:32 pm on April 28, 2025: none

    Closing as requested — changes were upstreamed and backported in #32358. Thanks everyone
18. tomasandroil closed this on Apr 28, 2025

    ---

Contributors
tomasandroil DrahtBot laanwj shahsb fanquake hebasto

Labels
Utils/log/libs