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

pull luke-jr wants to merge 3 commits into bitcoin:master from luke-jr:subproc_closefds changing 2 files +43 −1
  1. luke-jr commented at 8:11 pm on August 29, 2024: member

    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) since iterating all possible fd numbers has been found to be problematic.

    (Equivalent to #22417 was for boost::process)

  2. DrahtBot commented at 8:11 pm on August 29, 2024: 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/30756.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK laanwj

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

    Conflicts

    No conflicts as of last run.

  3. luke-jr force-pushed on Aug 29, 2024
  4. DrahtBot added the label CI failed on Aug 29, 2024
  5. luke-jr force-pushed on Aug 29, 2024
  6. Revert "remove unneeded close_fds option from cpp-subprocess"
    This reverts commit 79c30363733503a1fb7d4c98aa0d56ced0be6e32.
    790e2f948a
  7. luke-jr force-pushed on Aug 30, 2024
  8. luke-jr commented at 3:30 am on August 30, 2024: member
    (Tangent: CI passed with 1e6b0f3ec3a878efb96019f40185809ed2b0699e, but it appears there’s a CI bug making it fail with the equivalent 28f6d4fa7eb1b0da16ee940922c5c466da2ec0ec)
  9. DrahtBot removed the label CI failed on Aug 30, 2024
  10. DrahtBot added the label CI failed on Sep 29, 2024
  11. DrahtBot removed the label CI failed on Oct 1, 2024
  12. achow101 requested review from hebasto on Oct 15, 2024
  13. laanwj commented at 1:05 pm on October 24, 2024: member

    Concept ACK. Closing unnecessary fds before starting external commands is good practice.

    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

    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.

    optimises it by iterating over /proc/self/fd/

    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.

  14. laanwj requested review from laanwj on Oct 24, 2024
  15. cpp-subprocess: Iterate through /proc/self/fd for close_fds option 0cb1dc0be9
  16. run_command: Enable close_fds option to avoid lingering fds ff198b5ce5
  17. luke-jr force-pushed on Jan 8, 2025
  18. luke-jr commented at 3:10 pm on January 8, 2025: member
    Refactored to build a std::vector<int> just in case fs::directory_iterator uses a fd itself during iteration.
  19. DrahtBot added the label CI failed on Jan 8, 2025
  20. DrahtBot removed the label CI failed on Jan 8, 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-01-15 06:12 UTC

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