-
Notifications
You must be signed in to change notification settings - Fork 30
Handle fork(2) failures #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK 16ebae8. Thanks for catching this, and also fixing more missing includes
@@ -83,6 +85,9 @@ int SpawnProcess(int& pid, FdToArgsFn&& fd_to_args) | |||
} | |||
|
|||
pid = fork(); | |||
if (pid == -1) { | |||
throw std::system_error(errno, std::system_category(), "fork"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I didn't know system_error took a third string argument. It'd be good to start using this more places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, safe to ignore in this case. Just the pattern func1(x, func2_may_possibly_modify_x());
is better to be avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
system_error took a third string argument. It'd be good to start using this more places.
#23 Tell std::system_error() which function failed
Cheerz!
16ebae8 Handle fork(2) failures (Vasil Dimov) Pull request description: 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. ACKs for top commit: ryanofsky: Tested ACK 16ebae8. Thanks for catching this, and also fixing more missing includes Tree-SHA512: 72c9a6842772207e8617b7bfa69c2be3cde4888d1225391ee287510f0c5558ba5099369080a2dc638c2486161f21720f15f49477f897da5578ad8461a663489b
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.