Skip to content

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

Merged
merged 1 commit into from
Jan 21, 2020
Merged

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Jan 19, 2020

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.

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.
Copy link
Collaborator

@ryanofsky ryanofsky left a 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");
Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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!

ryanofsky added a commit that referenced this pull request Jan 21, 2020
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
@ryanofsky ryanofsky merged commit 16ebae8 into bitcoin-core:master Jan 21, 2020
@vasild vasild deleted the fork_error_handling branch January 24, 2020 08:21
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Jun 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants