Re: msacm32: Avoid attempts to convert data when available output is negative in acmStreamConvert

[Andrew Eikum]

This doesn't seem right to me. The test succeeds even without your
changes to msacm32/stream.c because cbDstLength != cbPreparedDstLength
in acmStreamConvert. I tried moving cbDstLength=-4 before
PrepareHeader, and both PrepareHeader and Convert succeed on my Win7
VM.

Can you look further into this?

Andrew

On Sun, Nov 22, 2015 at 11:46:33PM +0800, Bruno Jesus wrote:
> Signed-off-by: Bruno Jesus <00cpxxx(a)gmail.com>
>
> The game Lost Horizon passes a negative value in cbDstLength while
> calling acmStreamConvert, this means we can't do the conversion
> because there is no output buffer available. Cast required since the
> value is DWORD.
>
> Fixes https://bugs.winehq.org/show_bug.cgi?id=24723
> diff --git a/dlls/msacm32/stream.c b/dlls/msacm32/stream.c
> index bdfc3cc..67c5ce9 100644
> --- a/dlls/msacm32/stream.c
> +++ b/dlls/msacm32/stream.c
> @@ -88,20 +88,25 @@ MMRESULT WINAPI acmStreamConvert(HACMSTREAM has, PACMSTREAMHEADER pash,
>
> if ((was = ACM_GetStream(has)) == NULL) {
>          WARN("invalid handle\n");
> -	return MMSYSERR_INVALHANDLE;
> +        return MMSYSERR_INVALHANDLE;
>      }
>      if (!pash || pash->cbStruct < sizeof(ACMSTREAMHEADER)) {
>          WARN("invalid parameter\n");
> -	return MMSYSERR_INVALPARAM;
> +        return MMSYSERR_INVALPARAM;
>      }
>      if (!(pash->fdwStatus & ACMSTREAMHEADER_STATUSF_PREPARED)) {
>          WARN("unprepared header\n");
> -	return ACMERR_UNPREPARED;
> +        return ACMERR_UNPREPARED;
>      }
>
> pash->cbSrcLengthUsed = 0;
>      pash->cbDstLengthUsed = 0;
>
> +    if ((LONG) pash->cbDstLength < 0) {
> +        WARN("invalid output length\n");
> +        return MMSYSERR_INVALPARAM;
> +    }
> +
>      /* Note: the ACMSTREAMHEADER and ACMDRVSTREAMHEADER structs are of same
>       * size. some fields are private to msacm internals, and are exposed
>       * in ACMSTREAMHEADER in the dwReservedDriver array
> diff --git a/dlls/msacm32/tests/msacm.c b/dlls/msacm32/tests/msacm.c
> index 4c9dca0..3b5a446 100644
> --- a/dlls/msacm32/tests/msacm.c
> +++ b/dlls/msacm32/tests/msacm.c
> @@ -527,7 +527,7 @@ static void test_prepareheader(void)
>      WAVEFORMATEX dst;
>      MMRESULT mr;
>      ACMSTREAMHEADER hdr;
> -    BYTE buf[sizeof(WAVEFORMATEX) + 32], pcm[512], input[512];
> +    BYTE buf[sizeof(WAVEFORMATEX) + 32], pcm[2048], input[512];
>      ADPCMCOEFSET *coef;
>
> src = (ADPCMWAVEFORMAT*)buf;
> @@ -567,6 +567,7 @@ static void test_prepareheader(void)
>      mr = acmStreamOpen(&has, NULL, (WAVEFORMATEX*)src, &dst, NULL, 0, 0, 0);
>      ok(mr == MMSYSERR_NOERROR, "open failed: 0x%x\n", mr);
>
> +    memset(input, 0, sizeof(input));
>      memset(&hdr, 0, sizeof(hdr));
>      hdr.cbStruct = sizeof(hdr);
>      hdr.pbSrc = input;
> @@ -594,10 +595,37 @@ static void test_prepareheader(void)
>      ok(mr == MMSYSERR_NOERROR, "prepare failed: 0x%x\n", mr);
>      ok(hdr.fdwStatus == (ACMSTREAMHEADER_STATUSF_PREPARED | ACMSTREAMHEADER_STATUSF_DONE), "header wasn't prepared: 0x%x\n", hdr.fdwStatus);
>
> +    hdr.fdwStatus &= ~ACMSTREAMHEADER_STATUSF_DONE;
> +    mr = acmStreamConvert(has, &hdr, ACM_STREAMCONVERTF_BLOCKALIGN);
> +    ok(mr == MMSYSERR_NOERROR, "convert failed: 0x%x\n", mr);
> +    ok(hdr.fdwStatus & ACMSTREAMHEADER_STATUSF_DONE, "conversion was not done: 0x%x\n", hdr.fdwStatus);
> +
>      mr = acmStreamUnprepareHeader(has, &hdr, 0);
>      ok(mr == MMSYSERR_NOERROR, "unprepare failed: 0x%x\n", mr);
>      ok(hdr.fdwStatus == ACMSTREAMHEADER_STATUSF_DONE, "header wasn't unprepared: 0x%x\n", hdr.fdwStatus);
>
> +    memset(&hdr, 0, sizeof(hdr));
> +    hdr.cbStruct = sizeof(hdr);
> +    hdr.pbSrc = input;
> +    hdr.cbSrcLength = sizeof(input);
> +    hdr.pbDst = pcm;
> +    hdr.cbDstLength = sizeof(pcm);
> +
> +    mr = acmStreamPrepareHeader(has, &hdr, 0);
> +    ok(mr == MMSYSERR_NOERROR, "prepare failed: 0x%x\n", mr);
> +    ok(hdr.fdwStatus == ACMSTREAMHEADER_STATUSF_PREPARED, "header wasn't prepared: 0x%x\n", hdr.fdwStatus);
> +
> +    hdr.cbDstLength = -4; /* Test for Lost Horizon (bug 24723) */
> +    hdr.cbDstLengthUsed = 12345;
> +    mr = acmStreamConvert(has, &hdr, ACM_STREAMCONVERTF_BLOCKALIGN);
> +    ok(mr == MMSYSERR_INVALPARAM, "expected 11, got %d!\n", mr);
> +    ok(hdr.cbDstLengthUsed == 0, "expected 0, got %d\n", hdr.cbDstLengthUsed);
> +
> +    hdr.cbDstLength = sizeof(pcm);
> +    mr = acmStreamUnprepareHeader(has, &hdr, 0);
> +    ok(mr == MMSYSERR_NOERROR, "unprepare failed: 0x%x\n", mr);
> +    ok(hdr.fdwStatus == 0, "header wasn't unprepared: 0x%x\n", hdr.fdwStatus);
> +
>      mr = acmStreamClose(has, 0);
>      ok(mr == MMSYSERR_NOERROR, "close failed: 0x%x\n", mr);
>  }

msacm32: Avoid attempts to convert data when available output is negative in acmStreamConvert

[Bruno Jesus]

On Mon, Nov 23, 2015 at 10:28 PM, Andrew Eikum <aeikum(a)codeweavers.com> wrote:
> This doesn't seem right to me. The test succeeds even without your
> changes to msacm32/stream.c because cbDstLength != cbPreparedDstLength
> in acmStreamConvert. I tried moving cbDstLength=-4 before
> PrepareHeader, and both PrepareHeader and Convert succeed on my Win7
> VM.
>
> Can you look further into this?

Hi, Andrew. Thanks for the review, you are right! I thought my tests
were good but it turns out they didn't really test what I was
expecting. I'm trying to understand this better, I will probably
resend the tests alone next time.

Best wishes,
Bruno