Re: wine3d3: Check the destination recangle when for FastBlt().

[Stefan Dösinger]

Am Sonntag, 29. Juli 2007 21:28 schrieb Peter Dons Tychsen:
> Hello Wine!
>
> I have fixed a small bug in FastBlt(), which caused Wine to crash if the
> application tried to do FastBlt() to a surface using a bad destination
> setup. The problem is already fixed in normal Blt(), but was broken for
> FastBlt().
>
> This fixes a crash for the little game "Treasure Mole Winter Vacations":

This check should be in ddraw, not wined3d. ddraw, d3d8 and d3d9 have 
different ideas about such checks, that's why the check was moved from 
WineD3D to the client libs. Originally it was in LockRect, and when it was 
moved the check was lost in Blt and BltFast. There should be a check in ddraw 
already, maybe it is not sufficient.

Also please write a test case to verify that the fix is correct(look at 
dlls/ddraw/tests/dsurface.c). This will also prevent the problem from coming 
back. Treasure Mole broke after the ddraw rewrite because exactly this check 
was missing. I implemented it in LockRect, like in old ddraw, but I forgot to 
write a test for BltFast. Then a d3d9 game was broken because d3d9 LockRect 
behaved differently, a test was written for Lock(Rect) in ddraw, d3d8 and 
d3d9, and the range check moved to ddraw, d3d8 and d3d9 to be able to 
implement different checking in each version. Since I forgot to write a test 
when I originally fixed Treasure Mole the bug could creep back in again :-(

wine3d3: Check the destination recangle when for FastBlt().

[Peter Dons Tychsen]

On Sun, 2007-07-29 at 23:08 +0200, Stefan Dösinger wrote:
> Am Sonntag, 29. Juli 2007 21:28 schrieb Peter Dons Tychsen:
> > Hello Wine!
> >
> > I have fixed a small bug in FastBlt(), which caused Wine to crash if the
> > application tried to do FastBlt() to a surface using a bad destination
> > setup. The problem is already fixed in normal Blt(), but was broken for
> > FastBlt().
> >
> > This fixes a crash for the little game "Treasure Mole Winter Vacations":
>
> This check should be in ddraw, not wined3d. ddraw, d3d8 and d3d9 have 
> different ideas about such checks, that's why the check was moved from 
> WineD3D to the client libs. Originally it was in LockRect, and when it was 
> moved the check was lost in Blt and BltFast. There should be a check in ddraw 
> already, maybe it is not sufficient.
>
> Also please write a test case to verify that the fix is correct(look at 
> dlls/ddraw/tests/dsurface.c). This will also prevent the problem from coming 
> back. Treasure Mole broke after the ddraw rewrite because exactly this check 
> was missing. I implemented it in LockRect, like in old ddraw, but I forgot to 
> write a test for BltFast. Then a d3d9 game was broken because d3d9 LockRect 
> behaved differently, a test was written for Lock(Rect) in ddraw, d3d8 and 
> d3d9, and the range check moved to ddraw, d3d8 and d3d9 to be able to 
> implement different checking in each version. Since I forgot to write a test 
> when I originally fixed Treasure Mole the bug could creep back in again :-(

OK.
I will move the fix and re-submit.

/Pedro

wine3d3: Check the destination recangle when for FastBlt().

[Peter Dons Tychsen]

On Sun, 2007-07-29 at 23:08 +0200, Stefan Dösinger wrote:
> Am Sonntag, 29. Juli 2007 21:28 schrieb Peter Dons Tychsen:
> > Hello Wine!
> >
> > I have fixed a small bug in FastBlt(), which caused Wine to crash if the
> > application tried to do FastBlt() to a surface using a bad destination
> > setup. The problem is already fixed in normal Blt(), but was broken for
> > FastBlt().
> >
> > This fixes a crash for the little game "Treasure Mole Winter Vacations":
>
> This check should be in ddraw, not wined3d. ddraw, d3d8 and d3d9 have 
> different ideas about such checks, that's why the check was moved from 
> WineD3D to the client libs. Originally it was in LockRect, and when it was 
> moved the check was lost in Blt and BltFast. There should be a check in ddraw 
> already, maybe it is not sufficient.
>
> Also please write a test case to verify that the fix is correct(look at 
> dlls/ddraw/tests/dsurface.c). This will also prevent the problem from coming 
> back. Treasure Mole broke after the ddraw rewrite because exactly this check 
> was missing. I implemented it in LockRect, like in old ddraw, but I forgot to 
> write a test for BltFast. Then a d3d9 game was broken because d3d9 LockRect 
> behaved differently, a test was written for Lock(Rect) in ddraw, d3d8 and 
> d3d9, and the range check moved to ddraw, d3d8 and d3d9 to be able to 
> implement different checking in each version. Since I forgot to write a test 
> when I originally fixed Treasure Mole the bug could creep back in again :-(

Thanks for your comments Stefan,

I have taken another look at it. I still think this specific problem
should be fixed in wined3d. The problem occurs because BltFast (ddraw,
d3d and d3d-gdi) all take two DWORDs as the offset write position, and
then later casts them into a RECT structure which has signed values.
This must for any caller be considered an error. No caller could get
anything useful out of this.

Code:

IWineGDISurfaceImpl_BltFast(IWineD3DSurface *iface,
                            DWORD dstx,
                            DWORD dsty,
                            IWineD3DSurface *Source,
                            RECT *rsrc,
                            DWORD trans)

...

lock_dst.left = dstx;  <--- bad cast!
lock_dst.top = dsty; <--- bad cast!
lock_dst.right = dstx + w; <--- bad cast!
lock_dst.bottom = dsty + h; <--- bad cast!

Why does this not trigger a warning? Not sure.

I can still move the fix to ddraw, but as said, i am not sure it's the
right move.

Please comment,

/pedro

wine3d3: Check the destination recangle when for FastBlt().

[Stefan Dösinger]

> I have taken another look at it. I still think this specific problem
> should be fixed in wined3d. The problem occurs because BltFast (ddraw,
> d3d and d3d-gdi) all take two DWORDs as the offset write position, and
> then later casts them into a RECT structure which has signed values.
> This must for any caller be considered an error. No caller could get
> anything useful out of this.
> lock_dst.left = dstx;  <--- bad cast!
> lock_dst.top = dsty; <--- bad cast!
> lock_dst.right = dstx + w; <--- bad cast!
> lock_dst.bottom = dsty + h; <--- bad cast!
>
> Why does this not trigger a warning? Not sure.

Indeed this does not sound right. The unsigned to signed assignment doesn't 
look right. However, a problem should only occur if the highest bit of the 
DWORD is set, in which case this would be a very high value and would exeed 
the surface dimensions. Such a huge surface can't be created without 
exceeding the 2 GB userland VM size. Most likely the check in 
dlls/ddraw/surface.c, line 2067 runs into a signedness issue too.

wine3d3: Check the destination recangle when for FastBlt().

[Peter Dons Tychsen]

On Mon, 2007-07-30 at 22:18 +0200, Peter Dons Tychsen wrote:
> On Sun, 2007-07-29 at 23:08 +0200, Stefan Dösinger wrote:
> > Am Sonntag, 29. Juli 2007 21:28 schrieb Peter Dons Tychsen:
> > > Hello Wine!
> > >
> > > I have fixed a small bug in FastBlt(), which caused Wine to crash if the
> > > application tried to do FastBlt() to a surface using a bad destination
> > > setup. The problem is already fixed in normal Blt(), but was broken for
> > > FastBlt().
> > >
> > > This fixes a crash for the little game "Treasure Mole Winter Vacations":
> >
> > This check should be in ddraw, not wined3d. ddraw, d3d8 and d3d9 have 
> > different ideas about such checks, that's why the check was moved from 
> > WineD3D to the client libs. Originally it was in LockRect, and when it was 
> > moved the check was lost in Blt and BltFast. There should be a check in ddraw 
> > already, maybe it is not sufficient.
> >
> > Also please write a test case to verify that the fix is correct(look at 
> > dlls/ddraw/tests/dsurface.c). This will also prevent the problem from coming 
> > back. Treasure Mole broke after the ddraw rewrite because exactly this check 
> > was missing. I implemented it in LockRect, like in old ddraw, but I forgot to 
> > write a test for BltFast. Then a d3d9 game was broken because d3d9 LockRect 
> > behaved differently, a test was written for Lock(Rect) in ddraw, d3d8 and 
> > d3d9, and the range check moved to ddraw, d3d8 and d3d9 to be able to 
> > implement different checking in each version. Since I forgot to write a test 
> > when I originally fixed Treasure Mole the bug could creep back in again :-(

Thanks for your comments Stefan,

I have taken another look at it. I still think this specific problem
should be fixed in wined3d. The problem occurs because BltFast (ddraw,
d3d and d3d-gdi) all take two DWORDs as the offset write position, and
then later casts them into a RECT structure which has signed values.
This must for any caller be considered an error. No caller could get
anything useful out of this.

Code:

IWineGDISurfaceImpl_BltFast(IWineD3DSurface *iface,
                            DWORD dstx,
                            DWORD dsty,
                            IWineD3DSurface *Source,
                            RECT *rsrc,
                            DWORD trans)

...

lock_dst.left = dstx;  <--- bad cast!
lock_dst.top = dsty; <--- bad cast!
lock_dst.right = dstx + w; <--- bad cast!
lock_dst.bottom = dsty + h; <--- bad cast!

Why does this not trigger a warning? Not sure.

I can still move the fix to ddraw, but as said, i am not sure it's the
right move.

Please comment,

/pedro

Please note: this is a re-send. I think my mail-server trashed the first
one. Not sure what happened. Sorry if you get this more than once.