Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

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

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

  • Comments 28

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.

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

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

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

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

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

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

  • Dmitry: That's a start - you DO run out of threads in the pool quickly.

    What happens next?

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

  • SND_SENTRY | SND_SYSTEM ??? SND_NOSTOP?

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

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

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

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

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

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

Page 1 of 2 (28 items) 12