Handle fork(2) failures #22

pull vasild wants to merge 1 commits into bitcoin-core:master from vasild:fork_error_handling changing 2 files +10 −0

vasild commented at 8:13 PM on January 19, 2020: contributor

Generate() and SpawnProcess() call fork(2) but did not check if it failed (return -1) and continued execution with possible unexpected results.

Handle fork(2) failure by throwing an exception.

Handle fork(2) failures

Generate() and SpawnProcess() call fork(2) but did not check if it
failed (return -1) and continued execution with possible unexpected
results.

Handle fork(2) failure by throwing an exception.

16ebae8225

in src/mp/util.cpp:89 in 16ebae8225
```
  84 | @@ -83,6 +85,9 @@ int SpawnProcess(int& pid, FdToArgsFn&& fd_to_args)
  85 |      }
  86 |  
  87 |      pid = fork();
  88 | +    if (pid == -1) {
  89 | +        throw std::system_error(errno, std::system_category(), "fork");
```
ryanofsky commented at 6:27 PM on January 21, 2020:

Interesting, I didn't know system_error took a third string argument. It'd be good to start using this more places.
vasild commented at 11:43 AM on January 23, 2020:

Yes, it also occurred to me that if std::system_category() modifies errno then std::system_error() may get the wrong value of errno:

#include <errno.h> #include <stdio.h> int f1() { errno = 3; return 20; } void f2(int e, int x) { printf("%d %d\n", e, x); } int main(int, char**) { errno = 2; // resulted from fork() or whatever f2(errno, f1()); return 0; }

produces:

# clang 9.0.1 $ clang++90 t.cc -o t && ./t 2 20 # gcc 9.2.0 $ g++9 t.cc -o t && ./t 3 20
ryanofsky commented at 7:26 PM on January 23, 2020:

It's a possible concern, but in practice if that constructor was setting errno, it would probably break a lot of things. Also in the worst case error message text would just be less clear

vasild commented at 8:21 AM on January 24, 2020:

Yes, safe to ignore in this case. Just the pattern func1(x, func2_may_possibly_modify_x()); is better to be avoided.

vasild commented at 10:04 AM on February 21, 2020:

system_error took a third string argument. It'd be good to start using this more places.

https://github.com/chaincodelabs/libmultiprocess/pull/23 Tell std::system_error() which function failed

Cheerz!
ryanofsky approved
ryanofsky commented at 6:30 PM on January 21, 2020: collaborator

Tested ACK 16ebae8225ce3bf91ac0217704c12b67e5c81109. Thanks for catching this, and also fixing more missing includes
ryanofsky referenced this in commit 49a9637237 on Jan 21, 2020
ryanofsky merged this on Jan 21, 2020
ryanofsky closed this on Jan 21, 2020
vasild deleted the branch on Jan 24, 2020
bitcoin-core locked this on Jun 25, 2025

Contributors

vasild

ryanofsky

Linked (view graph)

#115 MSAN Error during connection shutdown #162 macos "Broken pipe" exceptions during tests #219 SIGABRT in ~ProxyClientBase with #29409 and rust client