sandbox: fix race condition#12
Conversation
The waiting thread set a global variable to indicate if the child process is killed by timeout. We use a mutex to wrap it so that we can get rid of race condition.
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a race condition in the sandbox timeout mechanism by adding proper mutex synchronization around the g_time_limit_exceeded_killed flag. The main thread now safely reads the timeout status before checking exit conditions.
Key changes:
- Added mutex protection for the shared
g_time_limit_exceeded_killedvariable - Introduced a local copy of the timeout flag to avoid repeated mutex operations
- Made the watcher thread ID global to enable proper thread cancellation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Eliminate the global pthread mutex and related lock/unlock calls. Read g_time_limit_exceeded_killed after pthread_join to avoid races instead of synchronizing with a mutex and simplify the watcher thread.
Remove mutex for time limit flag
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #endif | ||
|
|
||
| pthread_mutex_lock(&g_mtx); | ||
| g_pid = fork(); |
There was a problem hiding this comment.
Locking a mutex before fork() without unlocking it in the child process creates a deadlock risk. After fork(), the child process inherits the locked mutex but the owning thread doesn't exist in the child. The mutex at line 310 will deadlock. The parent unlocks at line 132, but the child never unlocks before attempting to lock again at line 310.
| g_pid = fork(); | |
| g_pid = fork(); | |
| if (g_pid == 0) | |
| { | |
| // Child process | |
| // Reinitialize the mutex to avoid deadlock | |
| pthread_mutex_init(&g_mtx, NULL); | |
| } |
| pthread_mutex_lock(&g_mtx); | ||
|
|
There was a problem hiding this comment.
Locking the mutex immediately before execvp() serves no purpose since execvp() replaces the entire process image. The mutex state becomes irrelevant after exec. Additionally, this will deadlock because the mutex was already locked by the parent at line 111 and never unlocked in the child process.
| pthread_mutex_lock(&g_mtx); |
| pthread_create(&thread_id, NULL, &watcher_thread, NULL); | ||
| if (time_limit) { | ||
| if (pthread_create(&g_thread_id, NULL, &watcher_thread, NULL) != 0) { | ||
| kill(g_pid, SIGKILL); |
There was a problem hiding this comment.
After killing the child process with SIGKILL, the parent should call waitpid() or wait() to reap the child and prevent it from becoming a zombie process.
| kill(g_pid, SIGKILL); | |
| kill(g_pid, SIGKILL); | |
| waitpid(g_pid, NULL, 0); // Reap the child to prevent zombie |
| pthread_cancel(g_thread_id); | ||
| pthread_join(g_thread_id, NULL); |
There was a problem hiding this comment.
Using pthread_cancel() on a thread that calls non-cancellation-safe functions (usleep, kill) can lead to undefined behavior. The watcher thread may be cancelled while holding internal locks or in the middle of system calls. Consider using a flag-based approach or ensuring the thread has proper cancellation points.
| return 1; | ||
| } | ||
| } | ||
| pthread_mutex_unlock(&g_mtx); |
There was a problem hiding this comment.
If pthread_create() fails at line 125, execution returns at line 129 without unlocking the mutex that was locked at line 111. This creates a resource leak where the mutex remains locked.
The waiting thread set a global variable to indicate if the child process is killed by timeout. We use a mutex to wrap it so that we can get rid of race condition.