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.

  • Whoops, yes, that's another thing to ignore.

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

  • 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?

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

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

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

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

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

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

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

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

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

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

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

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

  • Love these puzzles. :D

Page 2 of 3 (38 items) 123