Re: [PATCH] rasapi32: Ensure device count matches data returned

[Sebastian Lackner]

On 26.02.2017 20:41, Bruno Jesus wrote:
> From: Bruno Jesus <bjesus(a)codeweavers.com>
>
> Based on original patch by Qian Hong.
>
> Fixes bug https://bugs.winehq.org/show_bug.cgi?id=30378
>
> Signed-off-by: Bruno Jesus <bjesus(a)codeweavers.com>
> ---
>  dlls/rasapi32/rasapi.c       |  1 +
>  dlls/rasapi32/tests/rasapi.c | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/dlls/rasapi32/rasapi.c b/dlls/rasapi32/rasapi.c
> index fcd0cb8..f97d4c1 100644
> --- a/dlls/rasapi32/rasapi.c
> +++ b/dlls/rasapi32/rasapi.c
> @@ -254,6 +254,7 @@ DWORD WINAPI RasEnumDevicesA(LPRASDEVINFOA lpRasDevinfo, LPDWORD lpcb, LPDWORD l
>   if(lpRasDevinfo && lpRasDevinfo->dwSize != sizeof(RASDEVINFOA))
>       return ERROR_INVALID_SIZE;
>
> +	*lpcDevices = 1;
>   if (!lpRasDevinfo || (*lpcb < sizeof(RASDEVINFOA))) {
>       *lpcb = sizeof(RASDEVINFOA);
>       return ERROR_BUFFER_TOO_SMALL;
> diff --git a/dlls/rasapi32/tests/rasapi.c b/dlls/rasapi32/tests/rasapi.c
> index e0ff25f..e982f6c 100644
> --- a/dlls/rasapi32/tests/rasapi.c
> +++ b/dlls/rasapi32/tests/rasapi.c
> @@ -77,80 +77,104 @@ static void test_rasenum(void)
>
> /* test first parameter */
>      cb = bufsize;
> +    cDevices = 0xdead;
>      result = pRasEnumDevicesA(NULL, &cb, &cDevices);
>      ok(result == ERROR_BUFFER_TOO_SMALL ||
>      result == ERROR_INVALID_USER_BUFFER, /* win98 */
>      "Expected ERROR_BUFFER_TOO_SMALL, got %08d\n", result);
> +    ok(cDevices > 0 && cDevices != 0xdead, "Unexpected device count %d\n", cDevices);
>
> rasDevInfo[0].dwSize = 0;
>      cb = bufsize;
> +    cDevices = 0xdead;
>      result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices);
>      ok(result == ERROR_INVALID_SIZE ||
>      result == ERROR_INVALID_USER_BUFFER, /* win98 */
>      "Expected ERROR_INVALID_SIZE, got %08d\n", result);
> +    ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices);
>
> rasDevInfo[0].dwSize = sizeof(RASDEVINFOA) -1;
>      cb = bufsize;
> +    cDevices = 0xdead;
>      result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices);
>      ok(result == ERROR_INVALID_SIZE ||
>      result == ERROR_INVALID_USER_BUFFER, /* win98 */
>      "Expected ERROR_INVALID_SIZE, got %08d\n", result);
> +    ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices);
>
> rasDevInfo[0].dwSize = sizeof(RASDEVINFOA) +1;
>      cb = bufsize;
> +    cDevices = 0xdead;
>      result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices);
>      ok(result == ERROR_INVALID_SIZE ||
>      result == ERROR_INVALID_USER_BUFFER, /* win98 */
>      "Expected ERROR_INVALID_SIZE, got %08d\n", result);
> +    ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices);
>
> /* test second parameter */
>      rasDevInfo[0].dwSize = sizeof(RASDEVINFOA);
>      result = pRasEnumDevicesA(rasDevInfo, NULL, &cDevices);
> +    cDevices = 0xdead;

Whats the point to set this after the function call? And also, how is this
better than Qian Hongs original patch? Using %u is more correct for unsigned
values, and the empty line after the assignment of *lpcDevices also didn't
hurt.
> ok(result == ERROR_INVALID_PARAMETER,
>      "Expected ERROR_INVALID_PARAMETER, got %08d\n", result);
> +    ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices);
>
> rasDevInfo[0].dwSize = sizeof(RASDEVINFOA);
>      cb = 0;
>      result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices);
> +    cDevices = 0xdead;
>      ok(result == ERROR_BUFFER_TOO_SMALL ||
>      result == ERROR_INVALID_SIZE, /* vista, 2k8 */
>      "Expected ERROR_BUFFER_TOO_SMALL/ERROR_INVALID_SIZE, got %08d\n", result);
> +    ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices);
>
> rasDevInfo[0].dwSize = sizeof(RASDEVINFOA);
>      cb = bufsize -1;
>      result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices);
> +    cDevices = 0xdead;
>      ok(result == ERROR_BUFFER_TOO_SMALL,
>      "Expected ERROR_BUFFER_TOO_SMALL, got %08d\n", result);
> +    ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices);
>
> rasDevInfo[0].dwSize = sizeof(RASDEVINFOA);
>      cb = bufsize +1;
> +    cDevices = 0xdead;
>      result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices);
>      ok(result == ERROR_SUCCESS,
>      "Expected ERROR_SUCCESS, got %08d\n", result);
> +    ok(cDevices > 0 && cDevices != 0xdead, "Unexpected device count %d\n", cDevices);
>
> /* test third parameter */
>      rasDevInfo[0].dwSize = sizeof(RASDEVINFOA);
>      cb = bufsize;
> +    cDevices = 0xdead;
>      result = pRasEnumDevicesA(rasDevInfo, &cb, NULL);
>      ok(result == ERROR_INVALID_PARAMETER,
>      "Expected ERROR_INVALID_PARAMETER, got %08d\n", result);
> +    ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices);
>
> /* test combinations of invalid parameters */
> +    cDevices = 0xdead;
>      result = pRasEnumDevicesA(NULL, NULL, &cDevices);
>      ok(result == ERROR_INVALID_PARAMETER,
>      "Expected ERROR_INVALID_PARAMETER, got %08d\n", result);
> +    ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices);
>
> +    cDevices = 0xdead;
>      result = pRasEnumDevicesA(NULL, &cb, NULL);
>      ok(result == ERROR_INVALID_PARAMETER ||
>      result == ERROR_INVALID_USER_BUFFER, /* win98 */
>      "Expected ERROR_INVALID_PARAMETER, got %08d\n", result);
> +    ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices);
>
> cb = 0;
>      rasDevInfo[0].dwSize = 0;
> +    cDevices = 0xdead;
>      result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices);
>      ok(result == ERROR_INVALID_SIZE ||
>      broken(result == ERROR_BUFFER_TOO_SMALL), /* win98 */
>      "Expected ERROR_INVALID_SIZE, got %08d\n", result);
> +    ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices);
>
> HeapFree(GetProcessHeap(), 0, rasDevInfo);
>  }
> -- 2.9.3

[PATCH] rasapi32: Ensure device count matches data returned

[Bruno Jesus]

On Sun, Feb 26, 2017 at 5:11 PM, Sebastian Lackner
<sebastian(a)fds-team.de> wrote:
> On 26.02.2017 20:41, Bruno Jesus wrote:
> > From: Bruno Jesus <bjesus(a)codeweavers.com>
> >
> > Based on original patch by Qian Hong.
> >
> > Fixes bug https://bugs.winehq.org/show_bug.cgi?id=30378
> >
> > Signed-off-by: Bruno Jesus <bjesus(a)codeweavers.com>
> > ---
> >  dlls/rasapi32/rasapi.c       |  1 +
> >  dlls/rasapi32/tests/rasapi.c | 24 ++++++++++++++++++++++++
> >  2 files changed, 25 insertions(+)
> >
> > diff --git a/dlls/rasapi32/rasapi.c b/dlls/rasapi32/rasapi.c
> > index fcd0cb8..f97d4c1 100644
> > --- a/dlls/rasapi32/rasapi.c
> > +++ b/dlls/rasapi32/rasapi.c
> > @@ -254,6 +254,7 @@ DWORD WINAPI RasEnumDevicesA(LPRASDEVINFOA lpRasDevinfo, LPDWORD lpcb, LPDWORD l
> >       if(lpRasDevinfo && lpRasDevinfo->dwSize != sizeof(RASDEVINFOA))
> >               return ERROR_INVALID_SIZE;
> >
> > +     *lpcDevices = 1;
> >       if (!lpRasDevinfo || (*lpcb < sizeof(RASDEVINFOA))) {
> >               *lpcb = sizeof(RASDEVINFOA);
> >               return ERROR_BUFFER_TOO_SMALL;
> > diff --git a/dlls/rasapi32/tests/rasapi.c b/dlls/rasapi32/tests/rasapi.c
> > index e0ff25f..e982f6c 100644
> > --- a/dlls/rasapi32/tests/rasapi.c
> > +++ b/dlls/rasapi32/tests/rasapi.c
> > @@ -77,80 +77,104 @@ static void test_rasenum(void)
> >
> > /* test first parameter */
> >      cb = bufsize;
> > +    cDevices = 0xdead;
> >      result = pRasEnumDevicesA(NULL, &cb, &cDevices);
> >      ok(result == ERROR_BUFFER_TOO_SMALL ||
> >      result == ERROR_INVALID_USER_BUFFER, /* win98 */
> >      "Expected ERROR_BUFFER_TOO_SMALL, got %08d\n", result);
> > +    ok(cDevices > 0 && cDevices != 0xdead, "Unexpected device count %d\n", cDevices);
> >
> > rasDevInfo[0].dwSize = 0;
> >      cb = bufsize;
> > +    cDevices = 0xdead;
> >      result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices);
> >      ok(result == ERROR_INVALID_SIZE ||
> >      result == ERROR_INVALID_USER_BUFFER, /* win98 */
> >      "Expected ERROR_INVALID_SIZE, got %08d\n", result);
> > +    ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices);
> >
> > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA) -1;
> >      cb = bufsize;
> > +    cDevices = 0xdead;
> >      result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices);
> >      ok(result == ERROR_INVALID_SIZE ||
> >      result == ERROR_INVALID_USER_BUFFER, /* win98 */
> >      "Expected ERROR_INVALID_SIZE, got %08d\n", result);
> > +    ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices);
> >
> > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA) +1;
> >      cb = bufsize;
> > +    cDevices = 0xdead;
> >      result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices);
> >      ok(result == ERROR_INVALID_SIZE ||
> >      result == ERROR_INVALID_USER_BUFFER, /* win98 */
> >      "Expected ERROR_INVALID_SIZE, got %08d\n", result);
> > +    ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices);
> >
> > /* test second parameter */
> >      rasDevInfo[0].dwSize = sizeof(RASDEVINFOA);
> >      result = pRasEnumDevicesA(rasDevInfo, NULL, &cDevices);
> > +    cDevices = 0xdead;
>
> Whats the point to set this after the function call? And also, how is this
> better than Qian Hongs original patch? Using %u is more correct for unsigned
> values, and the empty line after the assignment of *lpcDevices also didn't
> hurt.

Hi, thanks for the review. That was certainly a copy & paste issue. I
see now that you have a staged patch for this that I completely missed
I don't know how, sunday hours wasted for me... The main difference to
Qian's patch is the place where lpcDevices = 1 is set but the staged
patch is already correct too. Please send your patch in ;-)