Playsound is failing on Vista! What's wrong?
Recently BillP, the author of the antispyware application WinPatrol asked on the MSDN forums about a problem he was having with his application.
His app called PlaySound(MAKEINTRESOURCE(IDR_WOOF), hInst, SND_RESOURCE | SND_SYNC | SND_NOWAIT) but it was failing (returning false).
He was wondering if this might be a bug in Vista's playsound implementation - a reasonable assumption given that his application worked just fine on previous versions of Windows.
I knew that we were passing all the PlaySound regression tests, and there are a number of elements of Windows that use SND_RESOURCE (the pearl startup sound is one of them), so the SND_RESOURCE functionality wasn't broken. I was puzzled for a bit and then I realized that there WAS one change to PlaySound in Vista (other than some general code cleanup, the addition of support for the SND_SENTRY and SND_SYSTEM flags and support for accessibility events on PlaySound).
For Vista, I tightened up the validation logic that's used when checking files before the PlaySound call. Among other things, we check to make sure that:
a) The cbSize field in the "fmt " tag in the WAV file is less than 1K in length and
b) The WAVEFORMATEX in the "fmt " tag in the WAV file fits inside the "fmt " tag (done by checking that the waveformatex->cbSize+sizeof(WAVEFORMATEX) is less than the size of the "fmt " chunk[1]).
I downloaded BillP's app, and checked the resources in the file. And in sure enough, the WAVEFORMATEX in the file had a length of 0x38 when it should have been 0. Once I patched his binary to change the 0x38 to a 0, his application stared barking away.
At this point there are two options: (1) BillP can fix his application to correct the corrupted resource or (2) Microsoft can change the PlaySound API to allow this kind of corruption (either to allow a bogus cbSize or to edit the app's WAVEFORMATEX to "fix" the bogus cbSize). Changing PlaySound is not trivial at this point, and it's not clear what the right fix is - the error might be in the size of the "fmt " chunk, which means that the information in the cbSize field might be accurate. In addition, the downstream audio rendering APIs are likely to choke on these malformed structures anyway.
It's this kind of subtle breaking change that makes modifying any of the older Windows APIs such a nightmare.
[1] We only check if the "fmt " chunk size is greater than sizeof(WAVEFORMAT) - if the "fmt " chunk is sizeof(WAVEFORMAT) than we assume that this structure is a WAVEFORMAT structure, which doesn't have a cbSize field.
Comments
Anonymous
July 24, 2007
It's pretty clear that there's a mandate and a precedence to be backward compatible with applications that were allowed to work in previous versions of Windows (outside of security additions to Vista). I would not think it unreasonable for that to apply here and Vista to be patched at some later date to support this feature that had existed prior. ...and add that check to the Microsoft application verifier when it supports those types of checks.Anonymous
July 24, 2007
So you're working on PlaySound() API ? Cool, I think that's one of the better and more useful Win32 APIs. I also like the wave* API set: simple, straight-forward, everything is as you expected and no surprises. Too bad there's no PlayMIDI() API (or has this been added to Vista ?), because for a lot of audio notifications, a short MIDI tune is enough and it's a lot smaller than a WAV file.Anonymous
July 24, 2007
It's great when old fogey's stick together. Larry was very kind to take the time to track this one down. I should point out that I was using another old fogey program called CoolEdit96 to create the WAV file used by WinPatrol. It's been a great tool but as you might guess from the name, it's over 10 year old. I voted for option 1 and will be releasing a new version of my application by the end of the month. It will make a lot of our fans happy to have Scotty barking again. Thanks again, Bill Pytlovany BillP StudiosAnonymous
July 24, 2007
> So you're working on PlaySound() API ? Past tense would be appropriate here. I would guess that whatever changes Larry made there were done in early 2006, maybe earlier. It takes a while for the snake to digest the rat. :) On this particular bug, I sympathize. The old saying is something like "be strict in what you produce and liberal in what you accept" but whoever said that didn't have to deal with security exploits. If the format is documentably bad then it's probably best to correct the format, not loosen the tolerance of PlaySound.Anonymous
July 24, 2007
The comment has been removedAnonymous
July 24, 2007
The comment has been removedAnonymous
July 24, 2007
And then when the next version of Windows adds even more validation, you need a new flag SND_STRICTER. Then in the next service pack, maybe SND_EVENSTRICTERSTILL, and then SND_YOUTHOUGHTWEWERESTRICTBEFORE.Anonymous
July 24, 2007
Oh no, it would have to follow the Framework Design Guidelines and be SND_STRICT2, SND_STRICT3, SND_STRICT4...Anonymous
July 24, 2007
The comment has been removedAnonymous
July 24, 2007
Sadly, this kind of error is quite common in media files. One culprit, I've found, is that DirectShow decoders tend to be quite lax with regard to validation and allow oddities like nBlockAlign=0. This then leads to mysterious failures when the Windows multimedia APIs reject the broken formats. The funniest error I ever saw was an AVI file that had a 300K BITMAPINFOHEADER in the video stream.Anonymous
July 24, 2007
An api, for when cat file.wav > /dev/audio is under-engineeredAnonymous
July 25, 2007
I don't see how one function call is any more over-engineered than /dev/audio. Presumably /dev/audio would have to do the same validation of input as PlaySound would.Anonymous
July 25, 2007
The comment has been removedAnonymous
July 25, 2007
I'm pretty sure that's what /dev/audio does. From what I remember, /dev/audio just accepted raw samples so the data had to be in a very specific format (something like 16-bit 8kHz mono or something, I don't remember exactly)Anonymous
July 26, 2007
Wow, That this is even being discussed is amazing. BillP is a LAZY PROGRAMMER. Make him fix his application. Period!! Reading this thread just reiterates the level of problems Microsoft has to deal with from Lazy Programmers and all the bad code they write, which Microsoft ultimately has to deal with (and in many cases get wrongly blamed for). For those that would say "well it worked before"... Too Bad, BillP was lazy and got lucky that it worked before. Well his luck ran just out, and now he actually needs to learn to code and fix his application.Anonymous
July 26, 2007
BillP was NOT a lazy programmer. His app had a bug, and it was a very subtle one.Anonymous
July 26, 2007
Lazy how? Bill likely didn't know what the WAVEFORMATEX had in it; likely just asking Microsoft Visual C++ to simply add a wav file to the projects resources. How is that being a "Lazy Programmer".Anonymous
July 26, 2007
Last sentence in the blog post reads "...than we assume...". I have many, many times seen the use of "then" where it should be "than". This is possibly the first I see the opposite. (English isn't even my first language) :-)Anonymous
July 30, 2007
The comment has been removedAnonymous
August 07, 2007
The comment has been removedAnonymous
September 13, 2007
In my last post , I enumerated a bewildering array of threats that the PlaySound API is subject to, todayAnonymous
September 17, 2007
It's been a long path, but we're finally at the point where I can finally present the threat model forAnonymous
September 17, 2007
It's been a long path, but we're finally at the point where I can finally present the threat