Skip to content
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

Potential race condition #82

Closed
NolanDeveloper opened this issue Mar 9, 2024 · 3 comments
Closed

Potential race condition #82

NolanDeveloper opened this issue Mar 9, 2024 · 3 comments

Comments

@NolanDeveloper
Copy link

According to Execution of signals signal handler may be called whenever there's transition between kernel and user mode. fork() is a syscall, so there should be such transition when it returns here. cleanup() function is installed for the number of termination signals before fork(). So it seems like it might be possible that cleanup() is called between the moment when fork() has finished and monitored_pid is assigned. This would make cleanup() to _exit() here. Then child process would continue running as long as it wants instead of being terminated.

I wasn't able to reproduce this issue.

@pixelb
Copy link
Member

pixelb commented Mar 10, 2024

Well I repro'd with:

-  monitored_pid = fork ();
+  int tpid = fork ();
+  if (tpid) sleep(10); /* delay monitor */
+  monitored_pid = tpid;

If you run timeout -v -- 30 sleep 60, and then kill the timeout process within 10s, the sleep remains

@pixelb
Copy link
Member

pixelb commented Mar 10, 2024

Actually I think I see a related race.
If fork() fails, monitored_pid is -1.
If a signal is received before the parent process exits, then a signal is sent to pid -1, which is a all processes for which we have permission to send signals :(

@pixelb
Copy link
Member

pixelb commented Mar 12, 2024

Fixed with commit ab4ffc8

@pixelb pixelb closed this as completed Mar 12, 2024
hubot pushed a commit that referenced this issue Mar 12, 2024
* src/timeout.c (main): Block cleanup signals earlier so that cleanup()
is not runnable until monitored_pid is in a deterministic state.
This ensures we always send a termination signal to the child
once it's forked.
* NEWS: Mention the bug fix.
Reported at #82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants