Re: winmm/tests: waveOutGetDevCaps called with invalid size

[Robert Reif]

Peter Åstrand wrote:
> Changelog:
>
> Calling waveOutGetDevCapsA/waveOutGetDevCapsW with size 4 is wrong, the 
> size should be the sizeof the structure used. waveOutGetDevCapsA needs to 
> succeed since the capsA structure is used later.
> Index: wave.c
>
> RCS file: /home/wine/wine/dlls/winmm/tests/wave.c,v
> retrieving revision 1.67
> diff -u -r1.67 wave.c
> --- wave.c	29 Feb 2008 12:18:36 -0000	1.67
> +++ wave.c	18 Apr 2008 08:43:54 -0000
> @@ -853,15 +853,13 @@
>         "expected, got %s\n",dev_name(device),wave_out_error(rc));
>      }
> - rc=waveOutGetDevCapsA(device,&capsA,4);
> - /* Final call must succeed, capsA will be used below */
> - rc=waveOutGetDevCapsA(device,&capsA,sizeof(capsA));
> ok(rc==MMSYSERR_NOERROR,
>    "waveOutGetDevCapsA(%s): MMSYSERR_NOERROR expected, got %s\n",
>    dev_name(device),wave_out_error(rc));
> -
> - rc=waveOutGetDevCapsW(device,&capsW,4);
> - ok(rc==MMSYSERR_NOERROR || rc==MMSYSERR_NOTSUPPORTED,
> -
>   "waveOutGetDevCapsW(%s): MMSYSERR_NOERROR or MMSYSERR_NOTSUPPORTED "
> -
>   "expected, got %s\n",dev_name(device),wave_out_error(rc));
> -
>
> if (rc!=MMSYSERR_NOERROR)
> -
>
> return;
>
> nameA=NULL;
> rc=waveOutMessage((HWAVEOUT)device, DRV_QUERYDEVICEINTERFACESIZE,
>
> Peter Åstrand		ThinLinc Chief Developer
> Cendio AB		http://www.cendio.se
> Wallenbergs gata 4
> 583 30 Linköping	Phone: [phone]

That's the whole point of the test, to check what happens with an 
invalid argument.

winmm/tests: waveOutGetDevCaps called with invalid size

[Peter Åstrand]

On Fri, 18 Apr 2008, Robert Reif wrote:
> > - rc=waveOutGetDevCapsA(device,&capsA,4);
> > - /* Final call must succeed, capsA will be used below */
> > - rc=waveOutGetDevCapsA(device,&capsA,sizeof(capsA));
> > ok(rc==MMSYSERR_NOERROR,
> >    "waveOutGetDevCapsA(%s): MMSYSERR_NOERROR expected, got %s\n",
> >    dev_name(device),wave_out_error(rc));
> > -
> > - rc=waveOutGetDevCapsW(device,&capsW,4);
> > - ok(rc==MMSYSERR_NOERROR || rc==MMSYSERR_NOTSUPPORTED,
> > -
> >   "waveOutGetDevCapsW(%s): MMSYSERR_NOERROR or MMSYSERR_NOTSUPPORTED "
> > -
> >   "expected, got %s\n",dev_name(device),wave_out_error(rc));
> > - if (rc!=MMSYSERR_NOERROR)
> > - return;
> That's the whole point of the test, to check what happens with an invalid
> argument.

So what do you think *should* happen with an invalid argument? The result 
is different on different Windows platforms. The behaviour is not 
specified by MSDN.

In any case, continuing and executing code that uses "capsA" even though 
the waveOutGetDevCapsA call failed is definitely wrong.

Rgds, 
---
Peter Åstrand		ThinLinc Chief Developer
Cendio AB		http://www.cendio.se
Wallenbergs gata 4
583 30 Linköping	Phone: [phone]

winmm/tests: waveOutGetDevCaps called with invalid size

[Robert Reif]

Peter Åstrand wrote:
> On Fri, 18 Apr 2008, Robert Reif wrote:
> > > - rc=waveOutGetDevCapsA(device,&capsA,4);
> > > - /* Final call must succeed, capsA will be used below */
> > > - rc=waveOutGetDevCapsA(device,&capsA,sizeof(capsA));
> > > ok(rc==MMSYSERR_NOERROR,
> > >    "waveOutGetDevCapsA(%s): MMSYSERR_NOERROR expected, got %s\n",
> > >    dev_name(device),wave_out_error(rc));
> > > -
> > > - rc=waveOutGetDevCapsW(device,&capsW,4);
> > > - ok(rc==MMSYSERR_NOERROR || rc==MMSYSERR_NOTSUPPORTED,
> > > -
> > >   "waveOutGetDevCapsW(%s): MMSYSERR_NOERROR or MMSYSERR_NOTSUPPORTED "
> > > -
> > >   "expected, got %s\n",dev_name(device),wave_out_error(rc));
> > > - if (rc!=MMSYSERR_NOERROR)
> > > - return;
> > That's the whole point of the test, to check what happens with an invalid
> > argument.
>
> So what do you think *should* happen with an invalid argument? The result 
> is different on different Windows platforms. The behaviour is not 
> specified by MSDN.
>
> In any case, continuing and executing code that uses "capsA" even though 
> the waveOutGetDevCapsA call failed is definitely wrong.
> Rgds,
>
> Peter Åstrand		ThinLinc Chief Developer
> Cendio AB		http://www.cendio.se
> Wallenbergs gata 4
> 583 30 Linköping	Phone: [phone]

This is the proper fix:

diff --git a/dlls/winmm/tests/wave.c b/dlls/winmm/tests/wave.c
index 60f0622..37367c2 100644
--- a/dlls/winmm/tests/wave.c
+++ b/dlls/winmm/tests/wave.c
@@ -888,6 +888,13 @@ static void wave_out_test_device(int dev
         nameA=strdup("not supported");
     }

+    rc=waveOutGetDevCapsA(device,&capsA,sizeof(capsA));
+    ok(rc==MMSYSERR_NOERROR,
+       "waveOutGetDevCapsA(%s): MMSYSERR_NOERROR expected, got %s\n",
+       dev_name(device),wave_out_error(rc));
+    if (rc!=MMSYSERR_NOERROR)
+        return;
+
     trace("  %s: "%s" (%s) %d.%d (%d:%d)\n",dev_name(device),capsA.szPname,
           (nameA?nameA:"failed"),capsA.vDriverVersion >> 8,
           capsA.vDriverVersion & 0xff, capsA.wMid,capsA.wPid);

winmm/tests: waveOutGetDevCaps called with invalid size

[Peter Åstrand]

On Mon, 21 Apr 2008, Robert Reif wrote:
> > > That's the whole point of the test, to check what happens with an invalid
> > > argument.
> >
> > So what do you think *should* happen with an invalid argument? The result is
> > different on different Windows platforms. The behaviour is not specified by
> > MSDN.
> >
> > In any case, continuing and executing code that uses "capsA" even though the
> > waveOutGetDevCapsA call failed is definitely wrong.
> This is the proper fix:

The fix for this part of the problem is fine. Please apply.

However, I still think it's VERY strange to expect success from an invalid 
call. You are describing it as an "invalid argument" yourself. Why should 
we expect a call with an "invalid argument" to succeed? And again, the 
test does NOT pass on all Microsoft versions; it fails on Terminal 
Servers.

If the point is to verify that waveOutGetDevCaps doesn't write more than 
the size supplied, this should be verified. I'd also like to point out 
that the Wine implementation also doesn't handle this correctly: Only 
waveOutGetDevCapsA does a memcpy() using the size argument; 
waveOutGetDevCapsW does not.

Rgds, 
---
Peter Åstrand		ThinLinc Chief Developer
Cendio AB		http://www.cendio.se
Wallenbergs gata 4
583 30 Linköping	Phone: [phone]

diff --git a/dlls/winmm/tests/wave.c b/dlls/winmm/tests/wave.c
index 60f0622..37367c2 100644
--- a/dlls/winmm/tests/wave.c
+++ b/dlls/winmm/tests/wave.c
@@ -888,6 +888,13 @@ static void wave_out_test_device(int dev
         nameA=strdup("not supported");
     }

+    rc=waveOutGetDevCapsA(device,&capsA,sizeof(capsA));
+    ok(rc==MMSYSERR_NOERROR,
+       "waveOutGetDevCapsA(%s): MMSYSERR_NOERROR expected, got %s\n",
+       dev_name(device),wave_out_error(rc));
+    if (rc!=MMSYSERR_NOERROR)
+        return;
+
     trace("  %s: "%s" (%s) %d.%d (%d:%d)\n",dev_name(device),capsA.szPname,
           (nameA?nameA:"failed"),capsA.vDriverVersion >> 8,
           capsA.vDriverVersion & 0xff, capsA.wMid,capsA.wPid);

winmm/tests: waveOutGetDevCaps called with invalid size

[Robert Reif]

Peter Åstrand wrote:
> However, I still think it's VERY strange to expect success from an 
> invalid call. You are describing it as an "invalid argument" yourself. 
> Why should we expect a call with an "invalid argument" to succeed? And 
> again, the test does NOT pass on all Microsoft versions; it fails on 
> Terminal Servers.

If size wasn't relevant, it wouldn't be passed.  There are only two 
options: only pass the number of bytes requested or return an error for 
unreasonable sizes.  What is Terminal Server doing?  You can check for 
the error return code if that's what it's doing and add it to the return 
values checked.  If it's doing something silly like ignoring the size 
and overrunning the buffer, then it's broken and needs to be black 
listed.  A page for the buffer can be allocated with a guard page after 
it to catch buffer overruns if necessary.
> If the point is to verify that waveOutGetDevCaps doesn't write more 
> than the size supplied, this should be verified. I'd also like to 
> point out that the Wine implementation also doesn't handle this 
> correctly: Only waveOutGetDevCapsA does a memcpy() using the size 
> argument; waveOutGetDevCapsW does not.

waveOutGetDevCapsW lets the driver do the copy (just like old versions 
of windows).  The driver should be checking the size and only copying 
that much.  If it doesn't, then it broken.

winmm/tests: waveOutGetDevCaps called with invalid size

[Peter Åstrand]

> > However, I still think it's VERY strange to expect success from an invalid
> > call. You are describing it as an "invalid argument" yourself. Why should we
> > expect a call with an "invalid argument" to succeed? And again, the test
> > does NOT pass on all Microsoft versions; it fails on Terminal Servers.
>
> If size wasn't relevant, it wouldn't be passed.

The size is used for determining which kind of WAVEOUTCAPS structure you 
pass.
> There are only two options:
> only pass the number of bytes requested or return an error for unreasonable
> sizes.  What is Terminal Server doing?

It's returning an error.
> > If the point is to verify that waveOutGetDevCaps doesn't write more than the
> > size supplied, this should be verified. I'd also like to point out that the
> > Wine implementation also doesn't handle this correctly: Only
> > waveOutGetDevCapsA does a memcpy() using the size argument;
> > waveOutGetDevCapsW does not.
>
> waveOutGetDevCapsW lets the driver do the copy (just like old versions of
> windows).  The driver should be checking the size and only copying that much.
> If it doesn't, then it broken.

Any references for this claim?

You are saying that the driver "should only copy that much", but *what* 
should it copy? In other words: If the size doesn't match the size of any 
known structure (WAVEOUTCAPSA/WAVEOUTCAPSW/WAVEOUTCAPS2A/WAVEOUTCAPS2W), 
should the driver then just *assume* some structure? Which one? As far as 
I know, there's no guarantee that, for example, the first 
sizeof(WAVEOUTCAPSW) bytes of WAVEOUTCAPSW are the same as WAVEOUTCAPS2W.

Rgds, 
---
Peter Åstrand		ThinLinc Chief Developer
Cendio AB		http://www.cendio.se
Wallenbergs gata 4
583 30 Linköping	Phone: [phone]

winmm/tests: waveOutGetDevCaps called with invalid size

[Robert Reif]

> > There are only two options:
> > only pass the number of bytes requested or return an error for unreasonable
> > sizes.  What is Terminal Server doing?
>
> It's returning an error.

And what is the error?  If you know the error code returned, Ill add it 
to the code so
we can end this discussion.
> > waveOutGetDevCapsW lets the driver do the copy (just like old versions of
> > windows).  The driver should be checking the size and only copying that much.
> > If it doesn't, then it broken.
>
> Any references for this claim?

Read the code.
> You are saying that the driver "should only copy that much", but *what* 
> should it copy? In other words: If the size doesn't match the size of any 
> known structure (WAVEOUTCAPSA/WAVEOUTCAPSW/WAVEOUTCAPS2A/WAVEOUTCAPS2W), 
> should the driver then just *assume* some structure? Which one? As far as 
> I know, there's no guarantee that, for example, the first 
> sizeof(WAVEOUTCAPSW) bytes of WAVEOUTCAPSW are the same as WAVEOUTCAPS2W.

Again, read the code. WAVEOUTCAPS2 is a superset of WAVEOUTCAPS
so it doesn't matter when the length is shorter than WAVEOUTCAPS.  The
drivers only support W so mixing A and W is not an issue.

winmm/tests: waveOutGetDevCaps called with invalid size

[Peter Åstrand]

> > > There are only two options: only pass the number of bytes requested 
> > > or return an error for unreasonable sizes.  What is Terminal Server 
> > > doing?
> >
> > It's returning an error.
>
> And what is the error?  If you know the error code returned, Ill add it 
> to the code so we can end this discussion.

The error is MMSYSERR_INVALPARAM.

Sure, you can add the error code to the list of accepted return code, but 
I still don't understand the meaning of this test. You are basically 
testing the sound card driver, rather than the winmm subsystem.
> > > waveOutGetDevCapsW lets the driver do the copy (just like old versions of
> > > windows).  The driver should be checking the size and only copying that
> > > much.
> > > If it doesn't, then it broken.
> >
> > Any references for this claim?
>
> Read the code.

Since we are trying to follow MS, reading the Wine code doesn't really 
give us a hint of what MS considers valid or invalid.
> > You are saying that the driver "should only copy that much", but *what*
> > should it copy? In other words: If the size doesn't match the size of any
> > known structure (WAVEOUTCAPSA/WAVEOUTCAPSW/WAVEOUTCAPS2A/WAVEOUTCAPS2W),
> > should the driver then just *assume* some structure? Which one? As far as I
> > know, there's no guarantee that, for example, the first sizeof(WAVEOUTCAPSW)
> > bytes of WAVEOUTCAPSW are the same as WAVEOUTCAPS2W.
>
> Again, read the code. WAVEOUTCAPS2 is a superset of WAVEOUTCAPS
> so it doesn't matter when the length is shorter than WAVEOUTCAPS.  The
> drivers only support W so mixing A and W is not an issue.

Are you saying that WODM_GETDEVCAPS messages are always limited to the "W" 
versions? So, drivers don't need to handle WODM_GETDEVCAPS messages with 
"A" type structs? I had no idea of that. Any reference for this?

Rgds, 
---
Peter Åstrand		ThinLinc Chief Developer
Cendio AB		http://www.cendio.se
Wallenbergs gata 4
583 30 Linköping	Phone: [phone]

winmm/tests: waveOutGetDevCaps called with invalid size

[Peter Åstrand]

> > And what is the error?  If you know the error code returned, Ill add it 
> > to the code so we can end this discussion.
>
> The error is MMSYSERR_INVALPARAM.

I can see that this patch has now been committed:

--- wine/dlls/winmm/tests/wave.c:1.67   Fri Feb 29 06:18:36 2008
+++ wine/dlls/winmm/tests/wave.c        Fri Apr 25 08:12:48 2008
@@ -854,9 +854,9 @@
     }

rc=waveOutGetDevCapsA(device,&capsA,4);
-    ok(rc==MMSYSERR_NOERROR,
-       "waveOutGetDevCapsA(%s): MMSYSERR_NOERROR expected, got %s\n",
-       dev_name(device),wave_out_error(rc));
+    ok(rc==MMSYSERR_NOERROR || rc==MMSYSERR_INVALPARAM,
+       "waveOutGetDevCapsA(%s): MMSYSERR_NOERROR or MMSYSERR_INVALPARAM "
+       "expected, got %s\n", dev_name(device),wave_out_error(rc));

However, the waveOutGetDevCapsW fails as well on WTS, also returning 
MMSYSERR_INVALPARAM.

We could add MMSYSERR_INVALPARAM as a valid return code, but again, if we 
accept both success and a bunch of failures, the test makes very little 
sense.

Here's a patch that completely removes the odd tests:

--- wave.c      25 Apr 2008 13:12:48 -0000      1.68
+++ wave.c      28 Apr 2008 06:57:07 -0000
@@ -853,16 +853,6 @@
        "expected, got %s\n",dev_name(device),wave_out_error(rc));
     }

-    rc=waveOutGetDevCapsA(device,&capsA,4);
-    ok(rc==MMSYSERR_NOERROR || rc==MMSYSERR_INVALPARAM,
-       "waveOutGetDevCapsA(%s): MMSYSERR_NOERROR or MMSYSERR_INVALPARAM "
-       "expected, got %s\n", dev_name(device),wave_out_error(rc));
-
-    rc=waveOutGetDevCapsW(device,&capsW,4);
-    ok(rc==MMSYSERR_NOERROR || rc==MMSYSERR_NOTSUPPORTED,
-       "waveOutGetDevCapsW(%s): MMSYSERR_NOERROR or MMSYSERR_NOTSUPPORTED 
"
-       "expected, got %s\n",dev_name(device),wave_out_error(rc));
-
     nameA=NULL;
     rc=waveOutMessage((HWAVEOUT)device, DRV_QUERYDEVICEINTERFACESIZE,
                       (DWORD_PTR)&size, 0);

Here's a patch that adds MMSYSERR_INVALPARAM as accepted return code:

--- wave.c      25 Apr 2008 13:12:48 -0000      1.68
+++ wave.c      28 Apr 2008 06:58:14 -0000
@@ -859,8 +859,8 @@
        "expected, got %s\n", dev_name(device),wave_out_error(rc));

rc=waveOutGetDevCapsW(device,&capsW,4);
-    ok(rc==MMSYSERR_NOERROR || rc==MMSYSERR_NOTSUPPORTED,
-       "waveOutGetDevCapsW(%s): MMSYSERR_NOERROR or MMSYSERR_NOTSUPPORTED 
"
+    ok(rc==MMSYSERR_NOERROR || rc==MMSYSERR_NOTSUPPORTED || rc==MMSYSERR_INVALPARAM,
+       "waveOutGetDevCapsW(%s): MMSYSERR_NOERROR or MMSYSERR_NOTSUPPORTED or MMSYSERR_INVALPARAM "
        "expected, got %s\n",dev_name(device),wave_out_error(rc));

nameA=NULL;

Rgds, 
---
Peter Åstrand		ThinLinc Chief Developer
Cendio AB		http://www.cendio.se
Wallenbergs gata 4
583 30 Linköping	Phone: [phone]

winmm/tests: waveOutGetDevCaps called with invalid size

[Robert Reif]

Peter Åstrand wrote:
> > > And what is the error?  If you know the error code returned, Ill add it 
> > > to the code so we can end this discussion.
> >
> > The error is MMSYSERR_INVALPARAM.
>
> I can see that this patch has now been committed:
>
> --- wine/dlls/winmm/tests/wave.c:1.67   Fri Feb 29 06:18:36 2008
> +++ wine/dlls/winmm/tests/wave.c        Fri Apr 25 08:12:48 2008
> @@ -854,9 +854,9 @@
>      }
>  rc=waveOutGetDevCapsA(device,&capsA,4);
> - ok(rc==MMSYSERR_NOERROR,
> -
>   "waveOutGetDevCapsA(%s): MMSYSERR_NOERROR expected, got %s\n",
> -
>   dev_name(device),wave_out_error(rc));
> - ok(rc==MMSYSERR_NOERROR || rc==MMSYSERR_INVALPARAM,
> -
>   "waveOutGetDevCapsA(%s): MMSYSERR_NOERROR or MMSYSERR_INVALPARAM "
> -
>   "expected, got %s\n", dev_name(device),wave_out_error(rc));
>
>
> However, the waveOutGetDevCapsW fails as well on WTS, also returning 
> MMSYSERR_INVALPARAM.
>
> We could add MMSYSERR_INVALPARAM as a valid return code, but again, if we 
> accept both success and a bunch of failures, the test makes very little 
> sense.
>
> Here's a patch that completely removes the odd tests:
>
> --- wave.c      25 Apr 2008 13:12:48 -0000      1.68
> +++ wave.c      28 Apr 2008 06:57:07 -0000
> @@ -853,16 +853,6 @@
>         "expected, got %s\n",dev_name(device),wave_out_error(rc));
>      }
> - rc=waveOutGetDevCapsA(device,&capsA,4);
> - ok(rc==MMSYSERR_NOERROR || rc==MMSYSERR_INVALPARAM,
> -
>   "waveOutGetDevCapsA(%s): MMSYSERR_NOERROR or MMSYSERR_INVALPARAM "
> -
>   "expected, got %s\n", dev_name(device),wave_out_error(rc));
> -
> - rc=waveOutGetDevCapsW(device,&capsW,4);
> - ok(rc==MMSYSERR_NOERROR || rc==MMSYSERR_NOTSUPPORTED,
> -
>   "waveOutGetDevCapsW(%s): MMSYSERR_NOERROR or MMSYSERR_NOTSUPPORTED 
>
> "
> -
>   "expected, got %s\n",dev_name(device),wave_out_error(rc));
> - nameA=NULL;
>    rc=waveOutMessage((HWAVEOUT)device, DRV_QUERYDEVICEINTERFACESIZE,
>                      (DWORD_PTR)&size, 0);
>
> Here's a patch that adds MMSYSERR_INVALPARAM as accepted return code:
>
> --- wave.c      25 Apr 2008 13:12:48 -0000      1.68
> +++ wave.c      28 Apr 2008 06:58:14 -0000
> @@ -859,8 +859,8 @@
>         "expected, got %s\n", dev_name(device),wave_out_error(rc));
>  rc=waveOutGetDevCapsW(device,&capsW,4);
> - ok(rc==MMSYSERR_NOERROR || rc==MMSYSERR_NOTSUPPORTED,
> -
>   "waveOutGetDevCapsW(%s): MMSYSERR_NOERROR or MMSYSERR_NOTSUPPORTED 
>
> "
> -
>
> ok(rc==MMSYSERR_NOERROR || rc==MMSYSERR_NOTSUPPORTED || rc==MMSYSERR_INVALPARAM,
> -
>   "waveOutGetDevCapsW(%s): MMSYSERR_NOERROR or MMSYSERR_NOTSUPPORTED or MMSYSERR_INVALPARAM "
>   "expected, got %s\n",dev_name(device),wave_out_error(rc));
>
> nameA=NULL;
> Rgds,
>
> Peter Åstrand		ThinLinc Chief Developer
> Cendio AB		http://www.cendio.se
> Wallenbergs gata 4
> 583 30 Linköping	Phone: [phone]

Let me get this straight, your company's product doesn't pass this wine 
regression test so you want to remove the test?

winmm/tests: waveOutGetDevCaps called with invalid size

[Peter Åstrand]

On Mon, 28 Apr 2008, Robert Reif wrote:
> > However, the waveOutGetDevCapsW fails as well on WTS, also returning
> > MMSYSERR_INVALPARAM.
> Let me get this straight, your company's product doesn't pass this wine
> regression test so you want to remove the test?

No. Quoting from my intial post:

"The CapsW test fails with our sound driver (tlsnd) as well as Microsofts
Terminal Services driver (rdpsnd)."

We can of course change our driver to accept invalid structure sizes, but 
"plain" Terminal Servers would still fail.

Of course, if you add MMSYSERR_INVALPARAM as accepted return code, as you 
suggest, the problem would go away, but then again, the test is mostly 
pointless, since it succeeds regardless of if the API call succeeds or 
not.

Rgds, 
---
Peter Åstrand		ThinLinc Chief Developer
Cendio AB		http://www.cendio.se
Wallenbergs gata 4
583 30 Linköping	Phone: [phone]

winmm/tests: waveOutGetDevCaps called with invalid size

[Robert Reif]

Peter Åstrand wrote:
> On Mon, 28 Apr 2008, Robert Reif wrote:
> > > However, the waveOutGetDevCapsW fails as well on WTS, also returning
> > > MMSYSERR_INVALPARAM.
> > Let me get this straight, your company's product doesn't pass this wine
> > regression test so you want to remove the test?
>
> No. Quoting from my intial post:
>
> "The CapsW test fails with our sound driver (tlsnd) as well as Microsofts
> Terminal Services driver (rdpsnd)."
>
> We can of course change our driver to accept invalid structure sizes, but 
> "plain" Terminal Servers would still fail.
>
> Of course, if you add MMSYSERR_INVALPARAM as accepted return code, as you 
> suggest, the problem would go away, but then again, the test is mostly 
> pointless, since it succeeds regardless of if the API call succeeds or 
> not.
> Rgds,
>
> Peter Åstrand		ThinLinc Chief Developer
> Cendio AB		http://www.cendio.se
> Wallenbergs gata 4
> 583 30 Linköping	Phone: [phone]

First of all, requesting 4 bytes is not invalid but a perfectly 
reasonable thing to do.
It is just reading the manufactures id and product id.  However this 
behavior
is not documented in MSDN so at best it's just unspecified behavior.  
The reason
most hardware drivers get this right is because Microsoft does check this
behavior in their hardware compatibility test so manufactures can get
certification that their hardware and drivers are working properly.  Now 
the
two drivers that are behaving differently happen to be software drivers and
have probably never had the hardware driver tests run on them.  Technically,
this behavior is not wrong because it's not specified but Microsoft 
would expect
that the drivers do the short copy without error if they wanted 
certification.

The tests in wine are valid but incomplete because they don't check for
the possible buffer overrun that could occur if the driver just ignores 
the requested
size.  A black list should be added for Terminal Server and your product
because they behave differently than most hardware drivers.  This would
ensure that regressions don't slip into wine when drivers are added or 
changed
and that wine behaves like Microsoft expects a hardware driver to behave.

winmm/tests: waveOutGetDevCaps called with invalid size

[Robert Reif]

Robert Reif wrote:
> Peter Åstrand wrote:
> > On Mon, 28 Apr 2008, Robert Reif wrote:
> > > > However, the waveOutGetDevCapsW fails as well on WTS, also returning
> > > > MMSYSERR_INVALPARAM.
> > > Let me get this straight, your company's product doesn't pass this wine
> > > regression test so you want to remove the test?
> >
> > No. Quoting from my intial post:
> >
> > "The CapsW test fails with our sound driver (tlsnd) as well as Microsofts
> > Terminal Services driver (rdpsnd)."
> >
> > We can of course change our driver to accept invalid structure sizes, but 
> > "plain" Terminal Servers would still fail.
> >
> > Of course, if you add MMSYSERR_INVALPARAM as accepted return code, as you 
> > suggest, the problem would go away, but then again, the test is mostly 
> > pointless, since it succeeds regardless of if the API call succeeds or 
> > not.
> > Rgds,
> >
> > Peter Åstrand		ThinLinc Chief Developer
> > Cendio AB		http://www.cendio.se
> > Wallenbergs gata 4
> > 583 30 Linköping	Phone: [phone]
>
> First of all, requesting 4 bytes is not invalid but a perfectly 
> reasonable thing to do.
> It is just reading the manufactures id and product id.  However this 
> behavior
> is not documented in MSDN so at best it's just unspecified behavior.  
> The reason
> most hardware drivers get this right is because Microsoft does check this
> behavior in their hardware compatibility test so manufactures can get
> certification that their hardware and drivers are working properly.  Now 
> the
> two drivers that are behaving differently happen to be software drivers and
> have probably never had the hardware driver tests run on them.  Technically,
> this behavior is not wrong because it's not specified but Microsoft 
> would expect
> that the drivers do the short copy without error if they wanted 
> certification.
>
> The tests in wine are valid but incomplete because they don't check for
> the possible buffer overrun that could occur if the driver just ignores 
> the requested
> size.  A black list should be added for Terminal Server and your product
> because they behave differently than most hardware drivers.  This would
> ensure that regressions don't slip into wine when drivers are added or 
> changed
> and that wine behaves like Microsoft expects a hardware driver to behave.

Another solution to this problem would be to add the equivlent of 
todo_windows.
That way we wouldn't have to throw out valid tests just because some random
driver in windows does the wrong thing.  That would ensure that wine works
properly even if windows doesn't.  I have seen the trend lately to throw out
valid tests that fail on windows due to driver dependent behavior and that
disturbs me.  I would like to see wine held up to a higher standard than 
typical
windows drivers.

winmm/tests: waveOutGetDevCaps called with invalid size

[Peter Åstrand]

On Tue, 29 Apr 2008, Robert Reif wrote:
> First of all, requesting 4 bytes is not invalid but a perfectly 
> reasonable thing to do. It is just reading the manufactures id and 
> product id.  However this behavior is not documented in MSDN so at best 
> it's just unspecified behavior.  The reason most hardware drivers get 
> this right is because Microsoft does check this behavior in their 
> hardware compatibility test so manufactures can get certification that 
> their hardware and drivers are working properly.  Now the two drivers

...

Ok, you have convinced me.
> The tests in wine are valid but incomplete because they don't check for 
> the possible buffer overrun that could occur if the driver just ignores 
> the requested size.  A black list should be added for Terminal Server 
> and your product because they behave differently than most hardware 
> drivers.  This would ensure that regressions don't slip into wine when 
> drivers are added or changed and that wine behaves like Microsoft 
> expects a hardware driver to behave.

Adding a black list for WTS sounds good to me; much better than just 
accepting MMSYSERR_INVALPARAM. GetSystemMetrics(SM_REMOTESESSION) can be 
used to detect WTS.

Rgds, 
---
Peter Åstrand		ThinLinc Chief Developer
Cendio AB		http://www.cendio.se
Wallenbergs gata 4
583 30 Linköping	Phone: [phone]