Re: Protect some DIB functions from bad inputs.

[Rein Klazes]

On Tue, 11 Apr 2006 10:24:14 +0200, you wrote:
> > Hi,
> >
> > There are a couple of entries in the bug database (at least #4334 and
> > #4664) where the application calculates a wrong pointer for bitmap data.
> > The application survives on Windows but crashes on wine.
> >
> > Changelog:
> > dlls/x11drv	: dib.c
> > dlls/gdi/tests	: bitmap.c
> > Protect the Set/Stretch DIBitfunctions from accessing bad bitmap data.
> > Add some tests that would crash without that.
>
> Hello,
>
> I'm sorry if writing to you directly is not right thing to do, I was
> advised so on #wine-hq.
>
> I would just like to ask you why this patch didn't get merged to git
> (and subsequently to 0.9.11) and if there is anything I can do to get it
> in in time for 0.9.12?

I have no idea why it was not merged, never got any comments. Cc' ed to
the developers list for suggestions. A re-diffed patch is attached.

Rein

Protect some DIB functions from bad inputs.

[Alexandre Julliard]

Rein Klazes  writes:
> I have no idea why it was not merged, never got any comments. Cc' ed to
> the developers list for suggestions. A re-diffed patch is attached.

IsBadReadPtr is broken and should never be used. You need to add an
exception handler around the actual access.

Protect some DIB functions from bad inputs.

[Vitaly Lipatov]

В сообщении от 12 апреля 2006 14:18 Alexandre Julliard написал(a):
> IsBadReadPtr is broken and should never be used. You need to add an
> exception handler around the actual access.

I know many MS's dlls use IsBadReadPtr for check pointers.
IsBadReadPtr is malfunction conceptually or it is not realized correctly yet?

Protect some DIB functions from bad inputs.

[Mike Hearn]

On Wed, 12 Apr 2006 14:44:18 +0400, Vitaly Lipatov wrote:
> I know many MS's dlls use IsBadReadPtr for check pointers.
> IsBadReadPtr is malfunction conceptually or it is not realized correctly yet?

The problem is you can't use it in thread safe code, because the pointer
may be correct when you test it and then go bad immediately afterwards. So
using it in Wine code is almost always a race condition.

There is another problem with it. It can blow away the stack guard page so
causing the stack to not grow correctly.

The thread-safe way to do this sort of thing is always to trap the error
if there is one. You can't know until you try whether it'll work or not,
in other words.

IsBad*Ptr has historically been used throughout Win32 to verify arguments,
but this was never a good idea and in Vista it has been "banned", which I
guess means Microsoft have gone through and removed the tests. Or at least
are not using them anymore for new APIs.

thanks -mike

Protect some DIB functions from bad inputs.

[Rolf Kalbermatter]

Mike Hearn [mailto:[e-mail]] wrote:
> IsBad*Ptr has historically been used throughout Win32 to verify arguments,
> but this was never a good idea and in Vista it has been "banned", which I
> guess means Microsoft have gone through and removed the tests. Or at least
> are not using them anymore for new APIs.
> From what I have seen I got the feeling that MS didn't use that function in

newer APIs for quite some time already. Having only dealt with a few DLLs I
can't be authoritive, but for instance I never came across it in shell32.dll
or urlmon.dll but it's all over the place in avicap32.dll, which seems to be
mostly a port from the original avicap.dll written for Win 3.1. Also some of
the VfW "device drivers" seem to make extensive use of this API.
It would make sense since in Win 3.1 you hadn't the same issues about functions
needing to be thread safe so there this function had some use and it was just
ported to Win32 to make existing software still load despite the fact that it
can't really provide the parameter checking it makes one believe to do.

Rolf Kalbermatter

Protect some DIB functions from bad inputs.

[Mike McCormack]

Vitaly Lipatov wrote:
> В сообщении от 12 апреля 2006 14:18 Alexandre Julliard написал(a):
> > IsBadReadPtr is broken and should never be used. You need to add an
> > exception handler around the actual access.
>
> I know many MS's dlls use IsBadReadPtr for check pointers.
> IsBadReadPtr is malfunction conceptually or it is not realized correctly yet?

Something about it here:

http://blogs.msdn.com/oldnewthing/archive/2006/01/17/513779.aspx

It also has a race condition.  A thread could unmap the memory your 
checked after you checked it and before you try to read it.

Mike

Protect some DIB functions from bad inputs.

[Vitaly Lipatov]

В сообщении от 12 апреля 2006 15:17 Mike McCormack написал(a):
> Vitaly Lipatov wrote:
> > В сообщении от 12 апреля 2006 14:18 Alexandre Julliard написал(a):
> > > IsBadReadPtr is broken and should never be used. You need to add an
> > > exception handler around the actual access.
> >
> > I know many MS's dlls use IsBadReadPtr for check pointers.
> > IsBadReadPtr is malfunction conceptually or it is not realized correctly
> > yet?
>
> Something about it here:
>
> http://blogs.msdn.com/oldnewthing/archive/2006/01/17/513779.aspx
>
> It also has a race condition.  A thread could unmap the memory your
> checked after you checked it and before you try to read it.
>
> Mike

Thanks for the link. I read it out and found message about change IsBadReadPtr 
behaviour in Windows Vista:
http://blogs.msdn.com/wndp/archive/2005/08/18/453124.aspx

Will we simplify IsBadReadPtr also?
I saw IsBadReadPtr is used in native OLE implementation. We have not to use it 
in our realization of OLE?
Well, I see:
> > В сообщении от 12 апреля 2006 14:18 Alexandre Julliard написал(a):
> > > IsBadReadPtr is broken

OK.