Repository navigation
Conversation
JobControl::armWatchdog started the watchdog thread and then move-assigned it into the job state without holding watchdog_mu. An on_expire callback that re-arms from the watchdog thread reads that handle in the self-re-arm guard. If the body thread was preempted for longer than the timeout between thread creation and the store, the guard read an empty handle, and the re-arm then joined or assigned over a live thread (std::terminate). The handle is now stored under watchdog_mu, which the watchdog thread takes before anything else. ThreadSanitizer reported the race in ProviderJobTest.RearmingTheWatchdogFromOnExpireIsANoOp on every run (20/20). With the fix it reports nothing in 20 runs, and descriptor_import_provider_job_test passes 19/19 under TSan.
…'s lock The guard held across the handle store was named lock, like the unique_lock the watchdog lambda declares, and GCC's -Wshadow rejected the build under -Werror. It is now store_lock; behaviour is unchanged.
Collaborator
Author
|
Superseded by #200, which fixes the same JobControl::armWatchdog race with a thread-local re-entry guard and shipped in 0.34.1. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Problem
JobControl::armWatchdog(pj_base/src/source/provider_job.cpp) started the watchdog thread and then move-assigned it intostate_.watchdogwithout holdingwatchdog_mu.An
on_expirecallback that re-arms runs on the watchdog thread and reads that handle in the self-re-arm guard (state_.watchdog.joinable() && get_id() == this_thread). Nothing ordered that read after the body thread's store. The only ordering was thestartedsemaphore, and it runs the other way: from the watchdog thread to the body thread.If the body thread is preempted for longer than the timeout between thread creation and the store:
stopWatchdog()skips the join.state_.watchdogwhile the body thread is doing the same. The result is a tornstd::thread, orstd::terminatefrom assigning over a joinable one.Fix
Store the handle while holding
watchdog_mu. The watchdog thread lockswatchdog_mubefore anything else, so its lateron_expireread happens after the handle is assigned. Thestartedsemaphore and the rest of the function are unchanged.Evidence
PJ_ENABLE_TSAN=ON, both variants built in the same container with the same flags:ProviderJobTest.RearmingTheWatchdogFromOnExpireIsANoOpreport the race atprovider_job.cpp:195against the store at:205(exit 66).descriptor_import_provider_job_testpasses 19/19.Release
This is a consumer-visible bug fix, so a PATCH.
CHANGELOG.mdgets a0.34.1section withHost contract: unchanged (no floor impact).VERSIONis left for the release commit.