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.

  • Answer to quick puzzle about security and synchronization

    http://blogs.msdn.com/oldnewthing/archive/2005/06/07/426296.aspx

  • Comment: Nope, that's not what's wrong.  But it's a great hint.

  • Well you better hope that "MyEventName" is unique....

  • Also, you have not set any security... so anyone can now set and reset the event.

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

    Now what's the second problem?  This one's much trickier.

  • It isn't that if "MyEventName" is already around you could open someone else's?

    Well there is also the terminal services issue, and where this event is shared.  Is it globally shared or only shared in the session?

    Depending on a bunch of things you could cause the main thread to wait forever.

  • Assume that MyEventName is unique.  And that both apps run in the same TS session (I should have added those to the "not a bug" parts - they're all valid concerns but not part of the issue).

    Here's another hint: I was VERY surprised when I figured this out.

  • Yeah the only thing I could see is if you opened someone else's event named the same thing, signaled it, they wake up reset the event to unsignaled, then the worker thread opens and waits forever.  Which causes your main thread to wait forever.

  • Jeff, as I said, this one's subtle, and horribly dangerous.  

    Assume that the events are unique.

  • ooooh.. no way... it couldn't be.  Could the second call to CreateEvent actually set the event to non-signaled?  MSDN says:

    "bInitialState

       [in] If this parameter is TRUE, the initial state of the event

       object is signaled; otherwise, it is nonsignaled. "

    If this is taken 100% to the text, then depending on the order of the calls and what runs when, the event could be set to unsignaled.

    Please tell me thats not true..... ouch...

  • That's not it either.  Read the MSDN documentation for CreateEvent more carefully.

  • The documentation says that "If lpEventAttributes is NULL, the event gets a default security descriptor. The ACLs in the default security descriptor for an event come from the primary or impersonation token of the creator. "

    Could this somehow cause a situation where the theard creates the event first, then main tries to, but fails because of secrutiy issues? Main would ends up with a null for the event, thus never signaling anything  and never starting the thread, deadlocking both of them.

  • Well the only thing I can see is that if the event is already created, then the bInitialState and bManualReset parameters are ignored.  This could cause an issue if the two threads opened the event differently, but they aren't in your example.

    Other than that, I'm stumped....

  • Tyler, as long as the client and server are running in the same account that won't happen (remember, this is a simplified example).

    Remember my comment: The second bug won't become obvious unless you've figured out the first.  Jeff figured out the first bug earlier in the thread.

    Also the second bug is very subtle (subtle enough that it surprised me when I realized it).

  • Does the problem have anything to do with the fact that CloseHandle isn't being called for any of the event handles, or is that something else we're supposed to ignore?

Page 1 of 3 (38 items) 123