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.

Comments (38)

  1. Comment says:

    Answer to quick puzzle about security and synchronization

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

  2. LarryOsterman says:

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

  3. JeffCurless says:

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

  4. JeffCurless says:

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

  5. LarryOsterman says:

    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.

  6. JeffCurless says:

    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.

  7. LarryOsterman says:

    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.

  8. JeffCurless says:

    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.

  9. LarryOsterman says:

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

    Assume that the events are unique.

  10. JeffCurless says:

    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…

  11. LarryOsterman says:

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

  12. Tyler says:

    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.

  13. JeffCurless says:

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

  14. LarryOsterman says:

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

  15. Michael Davis says:

    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?

  16. LarryOsterman says:

    Whoops, yes, that’s another thing to ignore.

  17. Ovidiu says:

    I venture to guess that one doesn’t even have to think about malicious code futzing with the application, just about two or more instances of the code running at the same time, in different processes.

    It looks like one app signaling the event will wake up everyone’s threads. That’s probably uncool since, except for the process setting the event, others might have a different view of the world at that time.

  18. JeffCurless says:

    Could it be that the worker thread gets created with ACLs from the default security descriptor from the primary token of the creator, and therefore will call CreateEvent with a different security descriptor than the first thread, since create event uses the ACLs in either the default or the impersonation token of the creator.  So if the first thread was impersonating you may, depending on order of calls, not be able to open the event?

  19. Dave says:

    Can there be multiple instances of the main program running? If so that could spell trouble.

  20. LarryOsterman says:

    Dave: There’s no "main" program running – just two peers.

    Ovidiu: I didn’t say that you didn’t have to worry about malicious code futzing with the application.  As Jeff mentioned above, the first problem is that you’re specifying a NULL DACL to the call to CreateEvent.

    Once you fix that, if you think about the problem right, the second problem shows up.

    Jeff, you’re on the wrong track.  Think about what happens after you’ve fixed the first problem.

  21. Jeremy Boschen says:

    The default DACL allows modification to the DACL and the event object, so anyone can open it and change the rights, thereby causing failure of the code that expects it to not change.  

  22. LarryOsterman says:

    Jeremy, yup, that’s the first problem.  JeffCurless pointed that out above.

    Once you fix that problem, you’ll see the second problem.

  23. Paul says:

    Well even if you modify the ACL, the specified users can still access the event; thereby, allowing malicious programs running as said users to access the event.

  24. LarryOsterman says:

    Paul: Why do you say that?  The ACL defines what rights are granted to the users of the object.

  25. Greg D says:

    I haven’t done much work with events, but does the surprise have something to do with the fact that the security descriptor in lpEventAttributes is ignored when the second CreateEvent() gets called on an event that already exists, resulting in an attempt to acquire EVENT_ALL_ACCESS?

    The docs call out such use as increasing the likelihood of a program’s requiring Administrator access (http://msdn2.microsoft.com/en-us/library/ms686670.aspx).  I don’t really know how that’d be a problem in the example code, though.

  26. Grant says:

    Well, if you set an DACL on the CreateEvents and get it wrong (e.g. generic rwx is insufficient) then the first CreateEvent works and the second doesn’t (access denied). In this case if the worker’s CreateEvent is scheduled first (Sleep helps), a deadlock results (the set never fires, the worker waits forever). Otherwise the worker exits with an error without executing the work.

  27. mirobin says:

    From MSDN:

    "If lpName matches the name of an existing named event object, this function requests the EVENT_ALL_ACCESS access right. "

    Odds are, when specifying the security attributes, you are only granting read/synchronize permissions (because you realize in your infinite wisdom that you don’t want some malicious process messing with your event).  In such a case, the second create call will fail.  

    Either your worker thread exits immediately (causing your application to exit immediately) or it "blocks" waiting for an event that never signals (meaning your application never exits).

  28. Paul says:

    Given that each of the peer processes is running as UserX. Since each peer needs access to the event, the event must be created such that UserX is granted the right to access the event.

    However, that doesn’t given exclusive event access to the peer processes. Any other process running as UserX will also have access to the event. In other words UserX will not be able to prevent itself from accessing its own objects.

    I haven’t really used ACLs before, so I might have misunderstood something. I’m assuming that the goal is to ensure that "authenticated" code is accessing the event. However, ACLs only limit access to users–not code.

    Then again maybe this is one of those "not a bug" comments.

  29. LarryOsterman says:

    DING!DING!DING!DING!  mirobin and Greg D nailed it.

    The problem is that the DACL you set has to grant EVENT_ALL_ACCESS rights to the user you want to access, even though all they really need is SYNCHRONIZE access.  That in turn makes it essentially impossible to set a meaningful ACL on the object.

  30. meh says:

    Love these puzzles. :D

  31. Sven Groot says:

    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. ;)

  32. Wedson says:

    That’s why CreateEventEx was introduced :)

  33. Arno says:

    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

  34. LarryOsterman says:

    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.

  35. 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…

  36. LarryOsterman says:

    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.

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

  38. Sven Groot says:

    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.