Fix missing error check in set_clo_on_exec for FD_CLOEXEC handling
#32342
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/32342. 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. |
|
@laanwj Thanks for the review! |
|
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 Please squash your commits and use a more informative commit message than "Update subprocess.h" . |
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.
178e8de to
b0415ab
Compare
|
@laanwj I've squashed the commits and updated the message as requested |
|
ACK b0415ab |
|
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. |
|
Changes look good.
Yes, please consider upstreaming the changes. |
|
@hebasto |
Thank you! I've backported the merged upstream PR in #32358. So this one can be closed, I think. |
|
Closing as requested — changes were upstreamed and backported in #32358. Thanks everyone |
cd95c9d subprocess: check and handle fcntl(F_GETFD) failure (Tomás Andróil) b7288de subprocess: Proper implementation of wait() on Windows (Haowen Liu) 7423214 subprocess: Do not escape double quotes for command line arguments on Windows (Hennadii Stepanov) bb9ffea subprocess: Explicitly define move constructor of Streams class (Shunsuke Shimizu) 174bd43 subprocess: Avoid leaking POSIX name aliases beyond `subprocess.h` (Hennadii Stepanov) 7997b76 subprocess: Fix cross-compiling with mingw toolchain (Hennadii Stepanov) 6476304 subprocess: Get Windows return code in wait() (Haowen Liu) d3f511b subprocess: Fix string_arg when used with rref (Haowen Liu) 2fd3f2f subprocess: Fix memory leaks (Haoran Peng) Pull request description: Most of these changes were developed during work on #29868 and #32342 and have since been upstreamed. As they are now merged, this PR backports them to our `src/util/subprocess.h` header. Required for #29868. A list of the backported PRs: - arun11299/cpp-subprocess#106 - arun11299/cpp-subprocess#110 - arun11299/cpp-subprocess#109 - arun11299/cpp-subprocess#99 - arun11299/cpp-subprocess#112 - arun11299/cpp-subprocess#107 - arun11299/cpp-subprocess#113 - arun11299/cpp-subprocess#116 - arun11299/cpp-subprocess#117 The following PRs were skipped for backporting: - arun11299/cpp-subprocess#108 because we are not planning to support this feature. - arun11299/cpp-subprocess#101 because that change has been already landed in #29849. ACKs for top commit: theStack: Light ACK cd95c9d laanwj: Code review re-ACK cd95c9d Tree-SHA512: f9b60b932957d2e1cad1d87f2ad8bb68c97136e9735eb78547018a42cc50c4652750367f29462eadb0512c27db1dd8a7d4b17a2f0aeab62b3dbf86db5f51a61c
Changes:
fcntl(fd, F_SETFD, flags);with a version that checks the return value.OSErroron 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_execin the future for completeness.