Fix a race condition during the shutdown of the worker process #5943
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a thread different from the main requests a shutdown through Initializer::requestShutdown, it should not call waitForShutdown; there's no reason to wait, moreover the function doesn't only wait, but also actually stops other components and then finally calls exit(). Since the main thread is already inside the waitForShutdown call waiting on Dispatcher::joinServices or inside the shutdown() callable on Windows, having a secondary thread do the same work potentially at the same time is wrong. Moreover calling exit() from a secondary thread is most of the time incorrect. The waitForShutdown function has been renamed to waitThenShutdown to better represent what it's actually doing.
|
The issue has been initially highlighted by running the test_osqueryd.py test, specifically the An example is in this build https://dev.azure.com/trailofbits/osquery/_build/results?buildId=1254 With TSAN help I've found that when killing the parent process, the worker, in a different thread than the main, notices that the watchdog is dead so requests a shutdown through requestShutdown(). Lines 756 to 771 in 33e5fcb which though not only waits, but also actually shutdowns various components. Moreover since Lines 727 to 741 in 33e5fcb and since WatcherWatcherRunner is itself a service added to the Dispatcher, the thread will end up joining on itself.
Another issue, ignoring joining on itself, is that waitForShutdown ends up calling exit(), from a secondary thread. Other useful information is that the main thread calls Lines 97 to 112 in 33e5fcb and waits on Dispatcher::joinServices, so the exit() will be called; it only needs to be waked up by something and then it will do the correct shutdown procedure. This is done already by requestShutdown(), which calls Dispatcher::stopServices(). |
|
Following the TSAN log, which shows that there's usage/destruction of the same object from two different threads: |
theopolis
requested changes
Oct 27, 2019
| raise(SIGUSR1); | ||
| } else { | ||
| // The main thread is requesting a shutdown, meaning in almost every case | ||
| // it is NOT waiting for a shutdown. | ||
| // Exceptions include: tight request / wait in an exception handler or | ||
| // custom signal handling. | ||
| Dispatcher::stopServices(); | ||
| waitForShutdown(); | ||
|
|
||
| if (current_thread_id == kMainThreadId) |
There was a problem hiding this comment.
Isn't this check superfluous? The else is only exercised if !(current_thread_id != kMainThreadId)
There was a problem hiding this comment.
The previous if doesn't only check for the thread id, but also if FLAGS_enable_signal_handler is enabled, which by default it's not.
This means that other threads can end up calling the else path, which caused the bug.
This is also shown by the TSAN callstack, where you have WatcherWatcherRunner entering in waitForShutdown through requestShutdown() and then calling exit().
theopolis
approved these changes
Oct 27, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a thread different from the main requests a shutdown
through Initializer::requestShutdown, it should not call
waitForShutdown; there's no reason to wait, moreover the function
doesn't only wait, but also actually stops other components and then
finally calls exit().
Since the main thread is already inside the waitForShutdown call
waiting on Dispatcher::joinServices or inside the shutdown() callable
on Windows, having a secondary thread do
the same work potentially at the same time is wrong.
Moreover calling exit() from a secondary thread is most of the time
incorrect.
The waitForShutdown function has been renamed to waitThenShutdown
to better represent what it's actually doing.