[PATCH v2] winmm: Don't crash in waveOutOpen when nSamplesPerSec is 0 and add tests

[Fabian Maurer]

Fixes bug 45530

v2: Fix test for XP and older

Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de>
---
 dlls/winmm/tests/wave.c | 38 ++++++++++++++++++++++++++++++++++++++
 dlls/winmm/waveform.c   |  6 ++++++
 2 files changed, 44 insertions(+)

diff --git a/dlls/winmm/tests/wave.c b/dlls/winmm/tests/wave.c
index b402e21917..cfc4c726be 100644
--- a/dlls/winmm/tests/wave.c
+++ b/dlls/winmm/tests/wave.c
@@ -1708,6 +1708,43 @@ static void test_PlaySound(void)
     DeleteFileA(test_file);
 }
 
+static void test_waveOutOpen_invalid_parameter(void)
+{
+    HWAVEOUT handle;
+    MMRESULT ret;
+    WAVEFORMATEX wfx;
+    WAVEFORMATEX const const_wfx = {
+        .wFormatTag = WAVE_FORMAT_PCM,
+        .nChannels = 1,
+        .nSamplesPerSec = 11025,
+        .nBlockAlign = 1,
+        .nAvgBytesPerSec = 11025 * 1,
+        .wBitsPerSample = 8,
+        .cbSize = 0
+    };
+
+    ret = waveOutOpen(&handle, 0, &const_wfx, 0, 0, 0);
+    if (ret != MMSYSERR_NOERROR)
+    {
+        skip("Could not to invalid parameter test\n");
+        return;
+    }
+    waveOutClose(handle);
+
+    /* Start actual tests */
+
+    wfx = const_wfx;
+    wfx.nAvgBytesPerSec = 0;
+    ret = waveOutOpen(&handle, 0, &wfx, 0, 0, 0);
+    ok(ret == MMSYSERR_NOERROR, "Got %d\n", ret);
+    waveOutClose(handle);
+
+    wfx = const_wfx;
+    wfx.nSamplesPerSec = 0;
+    ret = waveOutOpen(&handle, 0, &wfx, 0, 0, 0);
+    ok(ret == MMSYSERR_INVALPARAM || ret == WAVERR_BADFORMAT, "Got %d\n", ret); /* XP and lower return WAVERR_BADFORMAT */
+}
+
 START_TEST(wave)
 {
     test_multiple_waveopens();
@@ -1715,4 +1752,5 @@ START_TEST(wave)
     test_sndPlaySound();
     test_fragmentsize();
     test_PlaySound();
+    test_waveOutOpen_invalid_parameter();
 }
diff --git a/dlls/winmm/waveform.c b/dlls/winmm/waveform.c
index 045bf4ac20..58f830dad9 100644
--- a/dlls/winmm/waveform.c
+++ b/dlls/winmm/waveform.c
@@ -1106,6 +1106,12 @@ static LRESULT WINMM_OpenDevice(WINMM_Device *device, WINMM_OpenInfo *info,
             WARN("Fixing bad nAvgBytesPerSec (%u)\n", device->orig_fmt->nAvgBytesPerSec);
             device->orig_fmt->nAvgBytesPerSec  = device->orig_fmt->nSamplesPerSec * device->orig_fmt->nBlockAlign;
         }
+
+        if (info->format->nSamplesPerSec == 0)
+        {
+            ret = MMSYSERR_INVALPARAM;
+            goto error;
+        }
     }else{
         device->orig_fmt = HeapAlloc(GetProcessHeap(), 0,
                 sizeof(WAVEFORMATEX) + info->format->cbSize);
-- 
2.18.0

[Andrew Eikum]

Hi Fabian, some comments below.

On Sun, Jul 29, 2018 at 10:58:02PM +0200, Fabian Maurer wrote:
> Fixes bug 45530

We recently decided to use a standard format for bug numbers:

    https://wiki.winehq.org/Submitting_Patches#The_commit_message

e.g.:

    Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45530
> diff --git a/dlls/winmm/tests/wave.c b/dlls/winmm/tests/wave.c
> index b402e21917..cfc4c726be 100644
> --- a/dlls/winmm/tests/wave.c
> +++ b/dlls/winmm/tests/wave.c
> @@ -1708,6 +1708,43 @@ static void test_PlaySound(void)
>      DeleteFileA(test_file);
>  }
>
> +static void test_waveOutOpen_invalid_parameter(void)
> +{
> +    HWAVEOUT handle;
> +    MMRESULT ret;
> +    WAVEFORMATEX wfx;
> +    WAVEFORMATEX const const_wfx = {
> +        .wFormatTag = WAVE_FORMAT_PCM,
> +        .nChannels = 1,
> +        .nSamplesPerSec = 11025,
> +        .nBlockAlign = 1,
> +        .nAvgBytesPerSec = 11025 * 1,
> +        .wBitsPerSample = 8,
> +        .cbSize = 0
> +    };
> +

Unfortunately we care about ancient compilers, so we can't do struct
initialization like this.
> @@ -1715,4 +1752,5 @@ START_TEST(wave)
>      test_sndPlaySound();
>      test_fragmentsize();
>      test_PlaySound();
> +    test_waveOutOpen_invalid_parameter();

Instead of creating a new top-level test and using the 0th device, how
about putting this someplace in wave_out_test_device? It has a bunch
of format tests near the bottom, I think you could add the INVALPARAM
ones down there.
> diff --git a/dlls/winmm/waveform.c b/dlls/winmm/waveform.c
> index 045bf4ac20..58f830dad9 100644
> --- a/dlls/winmm/waveform.c
> +++ b/dlls/winmm/waveform.c
> @@ -1106,6 +1106,12 @@ static LRESULT WINMM_OpenDevice(WINMM_Device *device, WINMM_OpenInfo *info,
>              WARN("Fixing bad nAvgBytesPerSec (%u)\n", device->orig_fmt->nAvgBytesPerSec);
>              device->orig_fmt->nAvgBytesPerSec  = device->orig_fmt->nSamplesPerSec * device->orig_fmt->nBlockAlign;
>          }
> +
> +        if (info->format->nSamplesPerSec == 0)
> +        {
> +            ret = MMSYSERR_INVALPARAM;
> +            goto error;
> +        }

I think you could move this check up above the allocation and other
format checks. No need to do that work if the format is busted.

Thanks,
Andrew

[Fabian Maurer]

Thanks for the feedback,

I'll send in an updated patch.
Although I have to admit I  thought it was cleaner to open a new top 
level test for that behavior, that's how I learned it at least.

Regards,
Fabian Maurer