From e7717f2265b2bb9f63b4873b17f695b0089ed33d Mon Sep 17 00:00:00 2001 From: heinrich5991 Date: Tue, 5 Mar 2024 20:55:44 +0100 Subject: [PATCH] Fix race conditions in job state handling We want to change the state of the job from `STATE_QUEUED` to `STATE_RUNNING`, but not from `STATE_ABORTED`. Previously, the code usedd `exchange` to change the state and compared non-atomically afterwards. This led to `STATE_ABORTED` being replaced by `STATE_RUNNING` in a race. With `compare_exchange_strong`, this can no longer happen. Fixes #8044. --- src/engine/shared/jobs.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/engine/shared/jobs.cpp b/src/engine/shared/jobs.cpp index fb1b142c6..3c77e5fa6 100644 --- a/src/engine/shared/jobs.cpp +++ b/src/engine/shared/jobs.cpp @@ -83,9 +83,8 @@ void CJobPool::RunLoop() if(pJob) { - // do the job if we have one - const IJob::EJobState OldStateQueued = pJob->m_State.exchange(IJob::STATE_RUNNING); - if(OldStateQueued != IJob::STATE_QUEUED) + IJob::EJobState OldStateQueued = IJob::STATE_QUEUED; + if(!pJob->m_State.compare_exchange_strong(OldStateQueued, IJob::STATE_RUNNING)) { if(OldStateQueued == IJob::STATE_ABORTED) { @@ -109,10 +108,13 @@ void CJobPool::RunLoop() } // do not change state to done if job was not completed successfully - const IJob::EJobState OldStateRunning = pJob->m_State.exchange(IJob::STATE_DONE); - if(OldStateRunning != IJob::STATE_RUNNING) + IJob::EJobState OldStateRunning = IJob::STATE_RUNNING; + if(!pJob->m_State.compare_exchange_strong(OldStateRunning, IJob::STATE_DONE)) { - pJob->m_State = OldStateRunning; + if(OldStateRunning != IJob::STATE_ABORTED) + { + dbg_assert(false, "Job state invalid, must be either running or aborted"); + } } } else if(m_Shutdown)