What's wrong with this code, part 20(!): Yet another reason that named synchronization objects are dangerous...

When you're doing inter-process communication, it's often necessary to use named synchronization objects to communicate state between the processes.  For instance, if you have a memory section that's shared between two processes, it's often convenient to use a named mutex on both processes to ensure that only one process is accessing the memory at a time.

I recently had to fix a bug that was ultimately caused by a really common coding error when using named events.  I've taken the bug and stripped it down to just about the simplest form that still reproduced the error.

 

 const LPCWSTR EventName = L"MyEventName"; 
DWORD WINAPI WorkerThread(LPVOID context) 
{ 
    HANDLE eventHandle = CreateEvent(NULL, TRUE, FALSE, EventName); 
    if (eventHandle == NULL) 
    { 
        return GetLastError(); 
    } 

    WaitForSingleObject(eventHandle, INFINITE); 
    // Do Some Work. 
    return 0; 
} 

int _tmain(int argc, _TCHAR* argv[]) 
{ 
    HANDLE threadHandle = CreateThread(NULL, 0, WorkerThread, NULL, 0, NULL); 
    HANDLE eventHandle = CreateEvent(NULL, TRUE, FALSE, EventName); 

    SetEvent(eventHandle); 
    WaitForSingleObject(threadHandle, INFINITE); 
    return 0; 
}

There are actually TWO things wrong with this code, and they're both really bad.  The second one won't become apparent until after the first one's found.

 

Things that are not wrong:

  • Using CreateEvent in the worker thread instead of OpenEvent - remember, in the original incarnation of this code, the two components that deal with the event run in different processes - using CreateEvent allows you to protect against race conditions creating the event (if you call OpenEvent and follow it with a call to CreateEvent if the OpenEvent fails, there's a window where the other side could also call CreateEvent).
  • Calling CreateThread BEFORE calling CreateEvent - this was quite intentional to show off the potential for the race condition above.
  • There's limited error checking in this code.  While this code is not production quality, error handling could obfuscate the problem.
  • The _tmain/_TCHAR - this function's Unicode, VS stuck in the _T stuff in it's wizard.

 As always, kudos and explanations on Monday.