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

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)
    PlaySound(L".Default"NULL, SND_SYNC | SND_ALIAS);
    return 0;

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


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 (28)

  1. macbirdie says:

    I’d guess that if the default sound is long and you press many disallowed buttons, sounds will pile up and keep beeping even after you close the window.

    Also: "If a function in a DLL is queued to a worker thread, be sure that the function has completed execution before the DLL is unloaded."

    But maybe I’m not looking deep enough. 😉

  2. Gwyn says:

    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?

  3. Gwyn:  You’re close.  And you won the bonus points (that was the final fix that the team applied).

  4. GrayShade says:

    I’m not sure from the documentation — does QueueUserWorkItem() guarantee that the work item is not run on the calling thread?

  5. Dmitry says:

    You run out threads in the pool or out of memory very quickly

  6. Dave says:

    Seems like SND_ASYNC is the solution. Is there any reason you’d ever want to queue a sync PlaySound with WT_EXECUTELONGFUNCTION instead?

  7. Dmitry: That’s a start – you DO run out of threads in the pool quickly.

    What happens next?

  8. Daniel says:

    Nitpick: There’s a missing comma between ".Default" and NULL (not that it has any bearing on the actual problem).

  9. Alexandre Grigoriev says:


  10. Alexandre Grigoriev says:

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

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

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

  13. chaelim@gmail.com says:

    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.

  14. chaelim@gmail.com says:

    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.

  15. Sven says:

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

  16. Alexandre Grigoriev says:

    While we’re talking about audio, I’ll stray a little off topic.

    I always wanted to be able to monopolize audio in an active session and/or a selected application, shutting the background sessions out, in FUS environment. For example, when I’m using a media player or audio editing program, I don’t want to hear any other PlaySound crap. Or when there is another user’s session in the background, I don’t want to hear all those messenger dings. Is it possible at all?

  17. Alexandre Grigoriev says:

    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.

  18. feroze says:

    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?

  19. mpz says:


    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.

  20. Alexandre Grigoriev says:


    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.

  21. mpz says:

    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.

  22. Alexandre Grigoriev says:


    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.

  23. Alexandre: Yes any api can be abused.  And the User team in Windows has spent decades fighting against the abuse of SetForegroundWindow.

    We don’t want to enter into that kind of war of attenuation.

    There already is a "allow applications to take exclusive control of this device" checkbox, it’s on by default but it exists explicitly to deal with this scenario.

    There are several DAW applications and media playback applications that render in exclusive mode for the reasons you mentioned above (and to get bitperfect rendering and low latency rendering).

  24. Alexandre Grigoriev says:


    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.

  25. Alexandre: Those features are present in server SKUs as well as client SKUs.

  26. Jason says:

    Maybe I’m looking at this all wrong.  But the problem to be solved was "..to “beep” on invalid input.."

    What’s wrong with MessageBeep()?

    Isn’t PlaySound(_T(".Default"),…) a little obtuse to a future maintainer (or casual browser) of the code?  

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