Re: wined3d: Better validation of BltFast destination
Am Dienstag 22 Juni 2010 23:37:20 schrieb Iain Arnell:
--- a/dlls/wined3d/surface_base.c +++ b/dlls/wined3d/surface_base.c @@ -1657,6 +1657,11 @@ HRESULT WINAPI IWineD3DBaseSurfaceImpl_BltFast(IWineD3DSurface *iface, DWORD dst lock_dst.right = dstx + w; lock_dst.bottom = dsty + h;
+ if ( lock_dst.left < 0 || lock_dst.top < 0 ) { + WARN("Application gave us bad desitination offset for BltFast.\n"); + return WINEDDERR_INVALIDRECT; + } You're catching only the GDI surface case here since opengl surfaces don't use IWineD3DBaseSurfaceImpl_BltFast. I recommend to put this check into ddraw, like in your previous attempts(but keep checking for < 0, don't do any bit testing magic like in your old patches)
Am Mittwoch 23 Juni 2010 10:40:30 schrieb Stefan Dösinger:
You're catching only the GDI surface case here since opengl surfaces don't use IWineD3DBaseSurfaceImpl_BltFast. I recommend to put this check into ddraw, like in your previous attempts(but keep checking for < 0, don't do any bit testing magic like in your old patches)
Am Dienstag 22 Juni 2010 23:37:20 schrieb Iain Arnell:
Scratch this - after thinking again about Henri's hint, I've moved the fix to wined3d. I searched the archives to see if I missed any of Henri's suggestions but I couldn't find any like that. My consideration is that ddraw already has the other range checks, so it would just be consistent to keep it there. besides, in many situations ddraw, d3d8 and d3d9 have different checks or behavior in case of wrong parameters(see for example surface locking)
(ok, the latter is somewhat academic since only ddraw calls BltFast directly)
On Wed, Jun 23, 2010 at 10:47 AM, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
Am Mittwoch 23 Juni 2010 10:40:30 schrieb Stefan Dösinger:
You're catching only the GDI surface case here since opengl surfaces don't use IWineD3DBaseSurfaceImpl_BltFast. I recommend to put this check into ddraw, like in your previous attempts(but keep checking for < 0, don't do any bit testing magic like in your old patches)
I'd hope that the opengl implementation also has some checking of its own. I did the nasty bit twiddling to avoid making any assumptions about equivalence of types (is it always safe to assume that DWORD can be cast as a LONG?). And gcc optimizes dstx < 0 away as "comparison of unsigned expression < 0 is always false".
Am Dienstag 22 Juni 2010 23:37:20 schrieb Iain Arnell:
Scratch this - after thinking again about Henri's hint, I've moved the fix to wined3d. I searched the archives to see if I missed any of Henri's suggestions but I couldn't find any like that. My consideration is that ddraw already has the other range checks, so it would just be consistent to keep it there. besides, in many situations ddraw, d3d8 and d3d9 have different checks or behavior in case of wrong parameters(see for example surface locking)
(ok, the latter is somewhat academic since only ddraw calls BltFast directly)
Henri's magic words were simply "integer overflow". So I looked for the point where this actually happens. And I could make the same argument for making the check in wined3d - it also makes a whole bunch of range checks. But, of course, I'm willing to bow to your better judgement. -- Iain.
Iain Arnell wrote: > On Wed, Jun 23, 2010 at 10:47 AM, Stefan Dösinger > <stefandoesinger(a)gmx.at> wrote: >> Am Mittwoch 23 Juni 2010 10:40:30 schrieb Stefan Dösinger: >>> You're catching only the GDI surface case here since opengl surfaces don't >>> use IWineD3DBaseSurfaceImpl_BltFast. I recommend to put this check into >>> ddraw, like in your previous attempts(but keep checking for < 0, don't do >>> any bit testing magic like in your old patches) > > I'd hope that the opengl implementation also has some checking of its own. > > I did the nasty bit twiddling to avoid making any assumptions about > equivalence of types (is it always safe to assume that DWORD can be > cast as a LONG?). And gcc optimizes dstx < 0 away as "comparison of > unsigned expression < 0 is always false". In the Windows world: - DWORD == unsigned int32 - LONG == signed int32 bye michael
On 23 June 2010 10:40, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
I recommend to put this check into ddraw, like in your previous attempts(but keep checking for < 0, don't do any bit testing magic like in your old patches)
Both of you, please don't ever work on any kind of critical system. Seriously. Does the attached patch help?
On Wed, Jun 23, 2010 at 11:47 AM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 23 June 2010 10:40, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
I recommend to put this check into ddraw, like in your previous attempts(but keep checking for < 0, don't do any bit testing magic like in your old patches)
Both of you, please don't ever work on any kind of critical system. Seriously.
Does the attached patch help?
Ah! Yes, and thank you for the enlightenment. -- Iain.
participants (4)
-
Henri Verbeet -
Iain Arnell -
Michael Stefaniuc -
Stefan Dösinger