[PATCH v3 0/1] MR10966: dsound: Fail IDirectSoundCaptureBufferImpl_Lock() for invalid buffer range.
-- v3: dsound: Fail IDirectSoundCaptureBufferImpl_Lock() for invalid buffer range. https://gitlab.winehq.org/wine/wine/-/merge_requests/10966
From: Paul Gofman <pgofman@codeweavers.com> --- dlls/dsound/capture.c | 26 ++++++++------- dlls/dsound/tests/capture.c | 65 +++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 12 deletions(-) diff --git a/dlls/dsound/capture.c b/dlls/dsound/capture.c index dfb5e7d921c..2112f5dfe86 100644 --- a/dlls/dsound/capture.c +++ b/dlls/dsound/capture.c @@ -451,19 +451,21 @@ static HRESULT WINAPI IDirectSoundCaptureBufferImpl_Lock(IDirectSoundCaptureBuff EnterCriticalSection(&(This->device->lock)); if (This->device->client) { - *lplpvAudioPtr1 = This->device->buffer + dwReadCusor; - if ( (dwReadCusor + dwReadBytes) > This->device->buflen) { - *lpdwAudioBytes1 = This->device->buflen - dwReadCusor; - if (lplpvAudioPtr2) - *lplpvAudioPtr2 = This->device->buffer; - if (lpdwAudioBytes2) - *lpdwAudioBytes2 = dwReadBytes - *lpdwAudioBytes1; + if ( dwReadCusor >= This->device->buflen || !dwReadBytes || dwReadBytes > This->device->buflen) { + *lpdwAudioBytes1 = 0; + *lplpvAudioPtr1 = NULL; + if (lplpvAudioPtr2) + *lplpvAudioPtr2 = NULL; + if (lpdwAudioBytes2) + *lpdwAudioBytes2 = 0; + hres = DSERR_INVALIDPARAM; } else { - *lpdwAudioBytes1 = dwReadBytes; - if (lplpvAudioPtr2) - *lplpvAudioPtr2 = 0; - if (lpdwAudioBytes2) - *lpdwAudioBytes2 = 0; + *lplpvAudioPtr1 = This->device->buffer + dwReadCusor; + *lpdwAudioBytes1 = min(dwReadBytes, This->device->buflen - dwReadCusor); + if (lpdwAudioBytes2) + *lpdwAudioBytes2 = dwReadBytes - *lpdwAudioBytes1; + if (lplpvAudioPtr2) + *lplpvAudioPtr2 = dwReadBytes - *lpdwAudioBytes1 ? This->device->buffer : NULL; } } else { TRACE("invalid call\n"); diff --git a/dlls/dsound/tests/capture.c b/dlls/dsound/tests/capture.c index 63998276a77..004956e44c1 100644 --- a/dlls/dsound/tests/capture.c +++ b/dlls/dsound/tests/capture.c @@ -290,6 +290,71 @@ static BOOL capture_buffer_service(capture_state_t* state) if (rc!=DS_OK) return FALSE; + ptr1 = ptr2 = (void *)0xdeadbeef; + len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,(DWORD)-32,0,&ptr1,&len1,&ptr2,&len2,0); + ok(rc==DSERR_INVALIDPARAM,"IDirectSoundCaptureBuffer_Lock() failed: %08lx\n", rc); + ok(!ptr1 && !ptr2 && !len1 && !len2, "got %p, %lu, %p, %lu.\n", ptr1, len1, ptr2, len2); + + ptr1 = ptr2 = (void *)0xdeadbeef; + len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,(DWORD)-32,64,&ptr1,&len1,&ptr2,&len2,0); + ok(rc==DSERR_INVALIDPARAM,"IDirectSoundCaptureBuffer_Lock() returned: %08lx\n", rc); + ok(!ptr1 && !ptr2 && !len1 && !len2, "got %p, %lu, %p, %lu.\n", ptr1, len1, ptr2, len2); + + ptr1 = ptr2 = (void *)0xdeadbeef; + len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,(DWORD)-32,64,&ptr1,&len1,&ptr2,&len2,0); + ok(rc==DSERR_INVALIDPARAM,"IDirectSoundCaptureBuffer_Lock() returned: %08lx\n", rc); + ok(!ptr1 && !ptr2 && !len1 && !len2, "got %p, %lu, %p, %lu.\n", ptr1, len1, ptr2, len2); + + ptr1 = ptr2 = (void *)0xdeadbeef; + len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,0,0,&ptr1,&len1,&ptr2,&len2,0); + ok(rc==DSERR_INVALIDPARAM,"IDirectSoundCaptureBuffer_Lock() returned: %08lx\n", rc); + ok(!ptr1 && !ptr2 && !len1 && !len2, "got %p, %lu, %p, %lu.\n", ptr1, len1, ptr2, len2); + + ptr1 = ptr2 = (void *)0xdeadbeef; + len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,0,0,&ptr1,&len1,NULL,NULL,0); + ok(rc==DSERR_INVALIDPARAM,"IDirectSoundCaptureBuffer_Lock() returned: %08lx\n", rc); + ok(!ptr1 && !len1, "got %p, %lu.\n", ptr1, len1); + + ptr1 = ptr2 = (void *)0xdeadbeef; + len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,state->buffer_size,1,&ptr1,&len1,&ptr2,&len2,0); + ok(rc==DSERR_INVALIDPARAM,"IDirectSoundCaptureBuffer_Lock() returned: %08lx\n", rc); + ok(!ptr1 && !ptr2 && !len1 && !len2, "got %p, %lu, %p, %lu.\n", ptr1, len1, ptr2, len2); + + ptr1 = ptr2 = (void *)0xdeadbeef; + len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,state->buffer_size,1,&ptr1,&len1,&ptr2,&len2,0); + ok(rc==DSERR_INVALIDPARAM,"IDirectSoundCaptureBuffer_Lock() returned: %08lx\n", rc); + ok(!ptr1 && !ptr2 && !len1 && !len2, "got %p, %lu, %p, %lu.\n", ptr1, len1, ptr2, len2); + + ptr1 = ptr2 = (void *)0xdeadbeef; + len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,0,state->buffer_size + 1,&ptr1,&len1,&ptr2,&len2,0); + ok(rc==DSERR_INVALIDPARAM,"IDirectSoundCaptureBuffer_Lock() returned: %08lx\n", rc); + ok(!ptr1 && !ptr2 && !len1 && !len2, "got %p, %lu, %p, %lu.\n", ptr1, len1, ptr2, len2); + + ptr1 = ptr2 = (void *)0xdeadbeef; + len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,state->offset,state->buffer_size, + &ptr1,&len1,&ptr2,&len2,0); + ok(!rc,"IDirectSoundCaptureBuffer_Lock() returned: %08lx\n", rc); + if (state->offset) + ok(ptr1 == (char *)ptr2 + state->offset, "got %p, %p, buffer_size %ld, offset %ld.\n", + ptr1, ptr2, state->buffer_size, state->offset); + else + ok(!ptr2, "got %p.\n", ptr2); + ok(len1 == state->buffer_size - state->offset, "got %ld, buffer_size %ld, offset %ld.\n", + len1, state->buffer_size, state->offset); + ok(len2 == state->offset, "got %ld, buffer_size %ld, offset %ld.\n", + len2, state->buffer_size, state->offset); + rc=IDirectSoundCaptureBuffer_Unlock(state->dscbo,ptr1,len1,ptr2,len2); + ok(rc==DS_OK,"IDirectSoundCaptureBuffer_Unlock() returned: %08lx\n", rc); + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,state->offset,state->size, &ptr1,&len1,&ptr2,&len2,0); ok(rc==DS_OK,"IDirectSoundCaptureBuffer_Lock() failed: %08lx\n", rc); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10966
On Thu May 21 20:00:39 2026 +0000, Paul Gofman wrote:
Indeed, thanks, my added test only tests the case when start is invalid and the requested size is larger than the total buffer size, might behave differently for valid start and wraparound. I will test such case and update the patch accoringly. v3:
* add more tests and handle buffer wraparound (only fail if read cursor or read length is invalid). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10966#note_141013
On Thu May 21 21:50:42 2026 +0000, Paul Gofman wrote:
v3: * add more tests and handle buffer wraparound (only fail if read cursor or read length is invalid). As a separate note, I was initially adding some similar wraparound tests with passing NULL ptr2, len2 to Lock, but that mode seems buggy on Windows: passing NULL ptr2, len2 to _\\_\_Unlock after that leaves buffer in the buggy (still locked?) state and consequent locks start to fail. That is sort of consistent with what https://learn.microsoft.com/en-us/previous-versions/windows/desktop/ee418185...) says: "The second pointer is needed even if zero bytes were written to the second pointer.". But since we opted not to receive this pointer and length from Lock we don't have a straightforward way to have it. I tested that passing ptr2, len2 to Unlock obtained from previous lock (assuming that ptr2 can only be buffer start pointer or NULL which seems to be the case) and that fixed the failure on WIndows. But I didn't include these weirdness in the patch.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10966#note_141014
Matteo Bruni (@Mystral) commented about dlls/dsound/tests/capture.c:
+ len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,(DWORD)-32,0,&ptr1,&len1,&ptr2,&len2,0); + ok(rc==DSERR_INVALIDPARAM,"IDirectSoundCaptureBuffer_Lock() failed: %08lx\n", rc); + ok(!ptr1 && !ptr2 && !len1 && !len2, "got %p, %lu, %p, %lu.\n", ptr1, len1, ptr2, len2); + + ptr1 = ptr2 = (void *)0xdeadbeef; + len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,(DWORD)-32,64,&ptr1,&len1,&ptr2,&len2,0); + ok(rc==DSERR_INVALIDPARAM,"IDirectSoundCaptureBuffer_Lock() returned: %08lx\n", rc); + ok(!ptr1 && !ptr2 && !len1 && !len2, "got %p, %lu, %p, %lu.\n", ptr1, len1, ptr2, len2); + + ptr1 = ptr2 = (void *)0xdeadbeef; + len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,(DWORD)-32,64,&ptr1,&len1,&ptr2,&len2,0); + ok(rc==DSERR_INVALIDPARAM,"IDirectSoundCaptureBuffer_Lock() returned: %08lx\n", rc); + ok(!ptr1 && !ptr2 && !len1 && !len2, "got %p, %lu, %p, %lu.\n", ptr1, len1, ptr2, len2); These two tests look identical.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10966#note_141662
Matteo Bruni (@Mystral) commented about dlls/dsound/tests/capture.c:
+ len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,0,0,&ptr1,&len1,NULL,NULL,0); + ok(rc==DSERR_INVALIDPARAM,"IDirectSoundCaptureBuffer_Lock() returned: %08lx\n", rc); + ok(!ptr1 && !len1, "got %p, %lu.\n", ptr1, len1); + + ptr1 = ptr2 = (void *)0xdeadbeef; + len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,state->buffer_size,1,&ptr1,&len1,&ptr2,&len2,0); + ok(rc==DSERR_INVALIDPARAM,"IDirectSoundCaptureBuffer_Lock() returned: %08lx\n", rc); + ok(!ptr1 && !ptr2 && !len1 && !len2, "got %p, %lu, %p, %lu.\n", ptr1, len1, ptr2, len2); + + ptr1 = ptr2 = (void *)0xdeadbeef; + len1 = len2 = 0xdeadbeef; + rc=IDirectSoundCaptureBuffer_Lock(state->dscbo,state->buffer_size,1,&ptr1,&len1,&ptr2,&len2,0); + ok(rc==DSERR_INVALIDPARAM,"IDirectSoundCaptureBuffer_Lock() returned: %08lx\n", rc); + ok(!ptr1 && !ptr2 && !len1 && !len2, "got %p, %lu, %p, %lu.\n", ptr1, len1, ptr2, len2); Same for these two. Did the second instance intend to test with NULL `ptr2` and `len2`?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10966#note_141663
participants (3)
-
Matteo Bruni (＠Mystral) -
Paul Gofman
-
Paul Gofman (＠gofman)