Race conditions in critical.c?

[Fawzi Mohamed]

On 2008-09-25 23:23:10 +0200, Mike Hearn  <mike at plan99.net> said:

> Sorry, hit return too early. So is anybody going to take a look at 
> this? I just noticed there is a Bugzilla, should I file a bug?

You are right dcs->next should be set only after the initialization is 
performed.
do submit a bug on bugzilla
	http://d.puremagic.com/issues/

Well spotted!

Fawzi
> 
> Mike Hearn  Wrote:
> 
>> Mike Hearn Wrote:
>> 
>>> I was reading the code to the phobos runtime library and saw the 
>>> following code in critical.c:
>>> 
>>> void _d_criticalenter(D_CRITICAL_SECTION *dcs)
>>> {
>>> if (!dcs->next)
>>> {
>>> EnterCriticalSection(&critical_section.cs);
>>> if (!dcs->next) // if, in the meantime, another thread didn't set it
>>> {
>>> dcs->next = dcs_list;
>>> <----
>>> dcs_list = dcs;
>>> InitializeCriticalSection(&dcs->cs);
>>> }
>>> LeaveCriticalSection(&critical_section.cs);
>>> }
>>> EnterCriticalSection(&dcs->cs);
>>> }
>>> 
>>> There's a race condition here - consider a thread that is suspended at 
>>> the marked point. Another thread then attempts to lock the critsec. It 
>>> sees that dcs->next is set and calls EnterCriticalSection on an 
>>> uninitialized CRITICAL_SECTION.
>>> 
>>> The Linux code has the same problem, and then another one as well - the 
>>> initialization of the "critsec lock" is itself lazy and is not 
>>> protected by anything at all. The result is potentially multiple 
>>> initializations of that lock, and because atexit does not coalesce 
>>> multiple registrations, a potentially catastrophic multiple invocation 
>>> of _STD_critical_term.
>>> 
>>> It appears this code (for both platforms) needs to be scrapped and 
>>> rewritten. I'd start by checking that lazy initialization of critsecs 
>>> is actually needed - do D compilers really generate tons of potentially 
>>> unnecessary D_CRITICAL_SECTIONs?
>>> 
>>> thanks -mike