winealsa.drv:mmdevapi input validation

[Andrew Eikum]

Hi Joerg,

On 06/01/2011 07:00 AM, Joerg-Cyril.Hoehle at t-systems.com wrote:
> because of bug #27087, I had a look at the emerging mmdevapi code.
> Actually, it's the first time I read its documentation on MSDN.
> Here are a few comments of mine while reading the code.

Thanks for the careful review!

> The handling of duration (->  buffer size) in
> winealsa.drv/mmdevdrv.c:AudioClient_Initialize does not perform enough
> validation.  MSDN explicitly acknowledges that it could be 0.  I
> believe it should be related to ALSA's buffer, or a minimum of something
> like 2*period_size.  Or perhaps even 3*period size.

Yes. I have yet to see an application specify a 0 buffer, but we should 
handle it this way instead of failing. This is important and I'll see to 
fixing it soon.

> IIRC MSDN also documents a maximum of 2s for renderer and 0.5 for capture.

These limitations might be true for exclusive mode, which isn't really 
implemented in Wine. Frankly, I see little reason to spend time 
implementing special handling of exclusive mode unless we find an app 
that does something that requires it.

> AudioRenderClient_ReleaseBuffer does not check that written_frames is
> what GetBuffer returned.  Use with AUDCLNT_BUFFERFLAGS_SILENT and
> you'll have memcpy overwrite arbitrary amounts of memory.
> MSDN says: AUDCLNT_E_INVALID_SIZE

Yeah, that should be checked more closely.

> I believe Wine should currently include
>      if (AUDCLNT_STREAMFLAGS_EVENTCALLBACK&&  SHAREMODE_EXCLUSIVE)
>         { FIXME("unsupported")&  return E_ }
> because none of the buffer allocation and parameter checking specific
> to that mode that MSDN mentions in Initialize is implemented.

Is it really better to outright fail on that combination instead of 
having a (probably quite good) chance at just working? I think it would 
be worth having a loud FIXME when exclusive mode is requested, though.

> When it (the ping pong buffer logic) will be implemented, some better
> logic to trigger SetEvent() will probably be needed, because I'm
> unsure whether CreateTimerQueueTimer() provides the required stability
> over time and possibly ignores a laptop's suspend/resume cycle or xruns.
> In exclusive mode, reception of an event is a guarantee that
> GetBuffer(GetBufferSize frames) will succeed.

Hmm, perhaps. It would be nice to have an application that actually uses 
exclusive mode, to see how it behaves with this "non-exclusive" 
implementation. I have yet to see an application that uses exclusive 
mode, though...

> I suggest the values returned by AudioClient_GetDevicePeriod be adjusted
> to match what test.winehq shows.  10ms/3ms makes some sense, 10ms is
> mentioned a lot in MSDN and blogs.  3ms is small yet has a chance to
> work without glitch with MSDN's exclusive mode example.

These were tuned basically by hand, to values that worked well for the 
small number of test applications I had. I could try tweaking them to 
10ms/3ms and see if it still works. I agree that being consistent with 
Windows would be somewhat better arbitrary values than the ones we 
currently have.

> I can't remember seeing input validation of broken avgbytes and blockalign in the
> WAVEFORMAT parameter.  I showed some tests some month ago that prove that
> unlike Wine, winmm is not disturbed by bad values.  How does mmdevapi react to them?

I believe mmdevapi actually should validate those parameters, but I'm 
not certain. Tests would obviously clear that up.

None of the things above (except the 0 size buffer one) are really in 
need of urgent fixing, so I probably won't get to them for a while. I 
have made notes about them, though. If you would like to send patches 
fixing them (with tests, etc of course), please do. Notice that it's 
likely that the other drivers have the same problems.

Thanks again,
Andrew