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
Anonymous
May 11, 2007
Answer to quick puzzle about security and synchronization http://blogs.msdn.com/oldnewthing/archive/2005/06/07/426296.aspxAnonymous
May 11, 2007
Comment: Nope, that's not what's wrong. But it's a great hint.Anonymous
May 11, 2007
Well you better hope that "MyEventName" is unique....Anonymous
May 11, 2007
Also, you have not set any security... so anyone can now set and reset the event.Anonymous
May 11, 2007
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.Anonymous
May 11, 2007
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.Anonymous
May 11, 2007
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.Anonymous
May 11, 2007
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.Anonymous
May 11, 2007
Jeff, as I said, this one's subtle, and horribly dangerous. Assume that the events are unique.Anonymous
May 11, 2007
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...Anonymous
May 11, 2007
That's not it either. Read the MSDN documentation for CreateEvent more carefully.Anonymous
May 11, 2007
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.Anonymous
May 11, 2007
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....Anonymous
May 11, 2007
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).Anonymous
May 11, 2007
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?Anonymous
May 11, 2007
Whoops, yes, that's another thing to ignore.Anonymous
May 11, 2007
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.Anonymous
May 11, 2007
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?Anonymous
May 11, 2007
Can there be multiple instances of the main program running? If so that could spell trouble.Anonymous
May 11, 2007
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.Anonymous
May 11, 2007
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.Anonymous
May 11, 2007
Jeremy, yup, that's the first problem. JeffCurless pointed that out above. Once you fix that problem, you'll see the second problem.Anonymous
May 11, 2007
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.Anonymous
May 11, 2007
Paul: Why do you say that? The ACL defines what rights are granted to the users of the object.Anonymous
May 11, 2007
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.Anonymous
May 11, 2007
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.Anonymous
May 11, 2007
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).Anonymous
May 11, 2007
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.Anonymous
May 11, 2007
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.Anonymous
May 11, 2007
Love these puzzles. :DAnonymous
May 11, 2007
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. ;)Anonymous
May 11, 2007
That's why CreateEventEx was introduced :)Anonymous
May 12, 2007
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? ArnoAnonymous
May 12, 2007
The comment has been removedAnonymous
May 13, 2007
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...Anonymous
May 14, 2007
The comment has been removedAnonymous
May 14, 2007
Microsoft can be quite obsessive about instrumentation and metrics. We have a significant body of toolsAnonymous
May 14, 2007
The comment has been removed