Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

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

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

Rate This
  • Comments 38

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.

  • So the correct way to do this would be to call CreateEvent before calling CreateThread, and then to use OpenEvent in the thread so you can specify desired access? Although I suppose that would be impossible to do when it's really different processes and not different threads.

    And to be a hopeless nitpicker, the second problem is not actually a problem with the original code, since you're not setting a DACL there. ;)

  • That's why CreateEventEx was introduced :)

  • Larry, you said

    > Ah, Jeff hit the first part of the problem - there's no DACL set on the call to CreateEvent.

    The default DACL allows access to all processes this user started, and locks out all others, correct? I always programmed under the impression that this is fine security-wise, and that it is hard to do any better, so I happily left the DACL to NULL for anything I created.

    Apparently I am wrong. How can I do better? What is the DACL I should use?

    Arno

  • Wedson: Stop spoiling my fun :)

    Sven: Nope - that has other issues (there's a race condition that occurs if you do OpenEvent/CreateEvent - both sides could call OpenEvent and decide they had to create the event and then blam).

    Arno: Yes.  But the default DACL can be too permissive in many situations.

  • The documentation for CreateEventEx (at http://msdn2.microsoft.com/en-us/library/ms682400.aspx) still seems to say "If lpName matches the name of an existing named event object, this function requests the EVENT_ALL_ACCESS access right.", which would appear to be exactly the same problem.

    Is this a documentation bug, or am I not reading it right?

    Also, it'd be nice if the documentation for CreateEvent discussed what we're discussing now, and offered a pointer to the replacement function for use in new code...

  • Roger, it's a C&P error, I reported it to the doc people a couple of weeks ago, it should be fixed on the next doc update cycle.

  • Microsoft can be quite obsessive about instrumentation and metrics. We have a significant body of tools

  • Larry: I was aware of the race condition. That's why I said it would be impossible to use OpenEvent for different processes. Only if you're dealing with different threads on the same process it's (usually) possible since you can enforce which thread gets to create the event.

Page 3 of 3 (38 items) 123