Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

Playsound is failing on Vista! What's wrong?

Playsound is failing on Vista! What's wrong?

  • Comments 23

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. 

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

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

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

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

  • "Be liberal in what you accept and strict in what you emit" - engineering maxim designed to cause maximum grief.  ;)

    I'd guess that a lot of audio software had subtle bugs like that, and because the players had the corresponding loose validation, the bugs wern't detected - after all, if a few players accept technically invalid headers, who'll really notice that one byte in the header was wrong? When you improved the validation code that sort of issue was pretty much inevitable.

    If it was me, I'ld consider adding a "SND_STRICT" flag that enables the new better validation (and strongly encourage use of it in all future applications - especially if older software would just ignore it) and if it's not present then use the older more liberal validation code. Of course, even if it's possible to make that change (sounds like it's not easy) it then means an extra test and two validation paths in the Vista codebase, and people won't feel the same pressing need to improve their wave file creators.

    Adding a function, registry entry or application manifest field to make strict checking an application default is probably overkill - although I'ld certainly be tempted.

    If the downstream audio rendering APIs choke on the WAV yet it worked on the the older OS then clearly those APIs have also changed, and that's another kettle of fish.

    Another thought: in the PlaySound documentation I don't see any reference to GetLastError or any other way to determine why the function failed. Given multiple possibilities and the fact that finding the corrupted byte requires a reason to assume the file could be corrupted even though many WAV file players accept it happily some way to distinguish between different types of errors would be very useful.

    That's definately not what I'ld call a trivial change, but if you could have said "just check GetLastError()" and he said, "oh, it returned ERR_FILE_IS_CORRUPT_YOU_MORON" the issue would have been easier to solve - it would have been obvious to anyone that the validation code had changed.

  • Anony Moose, Unfortunately PlaySound doesn't return any error oher than "it worked" or "it didn't work".  It sucks.

    If we were to fix this, the likely fix would be to add a "SND_THEAPPISBROKEN" flag and turn that flag on in an appcompat shim - that way we'd only change the behavior for known apps.

    And I'm making changes to PlaySound today actually.

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

  • Oh no, it would have to follow the Framework Design Guidelines and be SND_STRICT2, SND_STRICT3, SND_STRICT4...

  • Broken code (and resources) are broken. Do not support them. The original version targeted pre-Vista. It still works there just fine. Some minor porting will be needed for future versions such as Vista. That is the developers' job, not Microsoft's. There are already too many hacks. Don't add any more.

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

  • An api, for when cat file.wav > /dev/audio is under-engineered

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

  • And I'd argue that it's far better to have the validation done in user mode instead of in /dev/audio.

    Personally I think that /dev/audio should just take raw audio samples and render them (leaving the question of what format should be used for rendering to the application).  It should NOT parse files and render the contents of those files.

    does cat foo.mp3 > /dev/audio also work?  

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

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

Page 1 of 2 (23 items) 12