Data race in wxThreadInternal on Windows

[egor.kazachkov]

Write-Read data race on m_state variable in wxWidgets::thread.cpp (lines 894, 437 for wxwidgets 2.8.12)
Write:
bool wxThreadInternal::Resume()
{

...
// don't change the state from STATE_EXITED because it's special and means
// we are going to terminate without running any user code - if we did it,
// the code in WaitForTerminate() wouldn't work
if ( m_state != STATE_EXITED )
{

m_state = STATE_RUNNING;

}

return true;

}

Read:

THREAD_RETVAL THREAD_CALLCONV wxThreadInternal::WinThreadStart(void *param)
{

THREAD_RETVAL rc = (THREAD_RETVAL)-1;

wxThread * const thread = (wxThread *)param;

// each thread has its own SEH translator so install our own a.s.a.p.
DisableAutomaticSETranslator();

// first of all, check whether we hadn't been cancelled already and don't
// start the user code at all then

const bool hasExited = thread->m_internal->GetState() == STATE_EXITED;
...
}

[vadz]

It looks like we need to protect this access to m_state with m_critsect, I'm not sure why isn't this being already done. Can you please confirm that just acquiring the critical section fixes the problem?

[egor.kazachkov]

Replying to vadz:

It looks like we need to protect this access to m_state with m_critsect, I'm not sure why isn't this being already done. Can you please confirm that just acquiring the critical section fixes the problem?

Yes, in build with added critical section protection there is no such issue.

[VZ]

(In [73125]) Add missing critical section locking before accessing shared variable.

WinThreadStart() in wxMSW wxThread implementation accessed the variable
containing the thread state without locking which was wrong, do it only inside
the critical section.

Notice that there is still an unavoidable race condition between exiting the
thread and starting it, so it's not clear at all if we should try to avoid
calling DoThreadStart() here.

Closes #14865.