What’s wrong with this code, part 26 – a real-world example

  • Article

This is an example of a real-world bug that was recently fixed in an unreleased Microsoft product.  I was told about the bug because it involved the PlaySound API (and thus they asked me to code review the fix), but it could happen with any application.

 static DWORD WINAPI _PlayBeep(__in void* pv)
{
    UNREFERENCED_PARAMETER(pv);
    PlaySound(L".Default"NULL, SND_SYNC | SND_ALIAS);
    return 0;
}

LRESULT WndProc(...)
{
    :
    :
    case WM_KEYDOWN:
        if (!_AcceptInputKeys(wParam, lParam))
        {
            QueueUserWorkItem(_PlayBeep, NULL, 0);
        }
        break;
}

 

This is actual code from inside the client side of a client/server component in Windows that was attempting to “beep” on invalid input (I’ve changed the code slightly to hide the actual origin and undoubtedly introduced issues).  And it has a whopper of a bug in it.

Given the simplicity of the code above, to get the answer right, it’s not enough to say what’s wrong with the code (the problem should be blindingly obvious).  You also need to be able to explain why this is so bad (in other words, what breaks when you do this).

 

Bonus points if you can identify the fix that was eventually applied.

Comments

  • Anonymous
    June 25, 2009
    The comment has been removed

  • Anonymous
    June 25, 2009
    I have to wonder why they used a worker thread and SND_SYNC, rather than forgoing the worker thread and using SND_ASYNC. Wouldn't that have the same effect and be simpler? Or is this an artifact of your refactoring?

  • Anonymous
    June 25, 2009
    Gwyn:  You're close.  And you won the bonus points (that was the final fix that the team applied).

  • Anonymous
    June 25, 2009
    I'm not sure from the documentation -- does QueueUserWorkItem() guarantee that the work item is not run on the calling thread?

  • Anonymous
    June 25, 2009
    You run out threads in the pool or out of memory very quickly

  • Anonymous
    June 25, 2009
    Seems like SND_ASYNC is the solution. Is there any reason you'd ever want to queue a sync PlaySound with WT_EXECUTELONGFUNCTION instead?

  • Anonymous
    June 25, 2009
    Dmitry: That's a start - you DO run out of threads in the pool quickly. What happens next?

  • Anonymous
    June 25, 2009
    Nitpick: There's a missing comma between ".Default" and NULL (not that it has any bearing on the actual problem).

  • Anonymous
    June 25, 2009
    SND_SENTRY | SND_SYSTEM ??? SND_NOSTOP?

  • Anonymous
    June 25, 2009
    Though the whole idea of making a default bleep, without an option to disable it, is wrong. Also, add SND_NODEFAULT flag. In "no sounds" scheme, I don't want it redirected to the beep.sys (XP volume slider in the taskbar forgot about it, and the speaker beep is so annoying).

  • Anonymous
    June 25, 2009
    My guess is that the PlaySound API uses an ansynchonrous procedure call (APC) when called with the synchronous flag and calls one of the wait routines waiting to be signaled that the sound has been completed by an event object or something.  Since QueueUserWorkItem is called with the default flags, there is no guarantee that the APC will be called and that the thread will return from the PlaySound function call, thus freezing one of the thread pool threads.  Play enough sounds and you'll run out of thread handles / resources / memory / etc. You probably changed behavior to utilize an existing or created a new thread that waits on an event for requests to play sounds and that is the thread that now executes the sound playback and informs the PlaySound thread that the sound has completed through another event object, etc.

  • Anonymous
    June 25, 2009
    Alex: snd_nostop would be bad :).  SND_ASYNC is the right answer. David's VERY close to what's going wrong - now think about the context of the application.

  • Anonymous
    June 25, 2009
    Total number of worker threads in the thread pool are increased (in worst case, up to 500 max probably). Creating thread itself expensive (makes CPU busy) and each thread takes 1MB (default value) virtual address space for the stack area that easily leads to out of memory crash later somewhere else.

  • Anonymous
    June 25, 2009
    After rethinking, one of the possibilities, if David is correct then , after reaching to the max total number of worker threads limit, eventually all worker threads will be frozen which means no work item will be processed and application hang? Just guessing.

  • Anonymous
    June 25, 2009
    Maybe it's the fact that the function is called _PlayBeep even though identifiers starting with an underscore and an uppercase letter are reserved to the implementation by the C and C++ standards? Yeah, I know that's not the bug but it's still a "problem" with the code. :)

  • Anonymous
    June 25, 2009
    The comment has been removed

  • Anonymous
    June 25, 2009
    OK, if PlaySound itself uses workitems, and they'll get to the same thread where _PlayBeep is executing, there will be a deadlock. In general, it's not a good idea to call functions that can wait from inside a workitem. Only lowest-level functions, such as file I/O are OK, and the file better be local.

  • Anonymous
    June 26, 2009
    Following up on David's comment - if a thread initiates async I/O, that thread has to be around for the I/O to complete, or the I/O gets cancelled. So, depending on how long the sound is going to take, if the WP thread exits before it is complet,e the sound playback will be dropped?

  • Anonymous
    June 26, 2009
    @Alexandre Programs would instantly start abusing this. "My sounds are more important than yours." and we'd go back to the Windows 3.x/95 times of "Sorry, somebody is already using the sound card." error messages.

  • Anonymous
    June 29, 2009
    @mpz, That should be controlled by the user. And this shouldn't pop any error messages. WAVE_MAPPER would just drop the sounds to the bit bucket.

  • Anonymous
    June 29, 2009
    User disables audio from all but one program by accident. User wonders why he doesn't get any audio, nor any error messages. And besides, Vista and 7 already support per-app volume settings. Feel free to tune down the apps you don't want to hear.

  • Anonymous
    June 29, 2009
    @mpz, I'm afraid you don't get it. The user would set a "allow this application to request exclusive use of audio" checkbox. Then the application will be allowed that. Also, if the user sets the "mute audio of the background sessions" checkbox, only the active session applications will pass the sound to the device. Per app volume settings are convenient, of course, but it would be too much hassle for the user to set them all to zero, and later restore back. Any API can be abused. SetForegroundWindow can be abused, and one of the worst abusers is Microsoft Internet Explorer.

  • Anonymous
    June 29, 2009
    The comment has been removed

  • Anonymous
    June 30, 2009
    (Shameless plug) Alexandre, here's a sample exclusive-mode player: http://blogs.msdn.com/matthew_van_eerde/archive/2009/04/03/sample-wasapi-exclusive-mode-event-driven-playback-app-including-the-hd-audio-alignment-dance.aspx

  • Anonymous
    June 30, 2009
    Larry, Thanks. Unforunately, I'm not playing that often with Vista/W7, mostly with the server SKU of those, and was not aware that the goodies have already arrived.

  • Anonymous
    June 30, 2009
    Alexandre: Those features are present in server SKUs as well as client SKUs.

  • Anonymous
    July 05, 2009
    The comment has been removed

  • Anonymous
    July 05, 2009
    Jason, there are some architectural issues with using MessageBeep for this application (it can't directly link with user32.dll for a number of reasons). And it doesn't affect the general principle (which is "don't overrun the worker thread pool because strange things could happen to your app").