Re: wine3d3: Check the destination recangle when for FastBlt().
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 :-(
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
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
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.
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.
participants (2)
-
Peter Dons Tychsen -
Stefan Dösinger