#14865 closed defect (fixed)

Data race in wxThreadInternal on Windows

Reported by: egor.kazachkov Owned by:
Priority: normal Milestone: 2.9.5
Component: wxMSW Version:
Keywords: thread Cc:
Blocked By: Blocking:
Patch: no

Description

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;

...
}

Change History (3)

comment:1 follow-up: Changed 22 months ago by vadz

  • Milestone set to 2.9.5

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?

comment:2 in reply to: ↑ 1 Changed 22 months ago by 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.

comment:3 Changed 22 months ago by VZ

  • Resolution set to fixed
  • Status changed from new to closed

(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.

Note: See TracTickets for help on using tickets.