Re: [ddraw] Fix bug 3487 take 2

[Christian Costa]

Peter Berg Larsen wrote:
> Walking backwards the bug was introduced in
>
> http://www.winehq.org/pipermail/wine-patches/2002-November/004161.html
>
> and altered in
>
> http://www.winehq.org/pipermail/wine-patches/2004-March/010091.html
>
> which states
> > Apps should initialize correctly the dwSize member of ddraw structures.
> > However some of them do not do this and Windows seems to handle that
> > case without crashing.
> > Here is a patch that prevents clearing more that the struct size.
> > This fixes the bug 2070.
>
> So here is better patch that does not open 2070 again.
>
> Changelog:
>         Bug in copying structs if to == from as to was memset first.
>
> diff -u Wine-20050930/dlls/ddraw/ddraw_private.h 
> Wine-20050930my/dlls/ddraw/ddraw_private.h
> --- Wine-20050930/dlls/ddraw/ddraw_private.h    2005-07-24 
> 18:17:29.000000000 +0200
> +++ Wine-20050930my/dlls/ddraw/ddraw_private.h    2005-10-09 
> 04:50:59.000000000 +0200
> @@ -41,18 +41,22 @@
>          (x)->dwSize = sizeof(*x);    \
>      } while (0)
>
> +/* __tosize can be set too large by some programs */
>  #define DD_STRUCT_COPY_BYSIZE(to,from)            \
>      do {                        \
> -
>        DWORD __size = (to)->dwSize;        \
> -
>        DWORD __copysize = __size;        \
> -
>        DWORD __resetsize = __size;        \
> -
>        if (__resetsize > sizeof(*to))        \
> -
>        __resetsize = sizeof(*to);        \
> -
>        memset(to,0,__resetsize);               \
> -
>        if ((from)->dwSize < __size)         \
> -
>        __copysize = (from)->dwSize;    \
> -
>    memcpy(to,from,__copysize);        \
> -
>    (to)->dwSize = __size;/*restore size*/    \
> -
>    DWORD __tosize = (to)->dwSize;        \
> -
>    DWORD __fromsize = (from)->dwSize;    \
> -
>    if ((to) == (from))            \
> -
>        break;                \
> -
>    if (__tosize > sizeof(*(to)))        \
> -
>        ERR("To struct's size too large");    \
> -
>    if (__fromsize > sizeof(*(from)))    \
> -
>        ERR("From struct's size too large");\
> -
>    if (__fromsize > __tosize)        \
> -
>        ERR("Copying too large struct");    \
> -
>    memcpy(to,from,__fromsize);        \
> -
>    memset(to+__fromsize,0,sizeof(*(to))-__fromsize);    \
> -
>    (to)->dwSize = __tosize;/*restore size*/\
> } while (0)
>
> #define MAKE_FOURCC(a,b,c,d) ((a << 0) | (b << 8) | (c << 16) | (d << 
> 24))

This does not work if the destination size is smaller than the struct 
size or source size is greater than destination size.
Even worst, if source size is greater than struct size, 
sizeof(*(to))-__fromsize is < 0.
Why don't you just add the "if (to = from) break;" statement ?

Christian

[ddraw] Fix bug 3487 take 2

[Peter Berg Larsen]

On Sun, 9 Oct 2005, Lionel Ulmer wrote:
> Well, while your patch was lying in the moderation queue, I sent what I 
> feel is a better solution to this problem (which fixes also a severe 
> reference counting issue).

I had more than one goal with the patch, more below.
> Could you try it and tell if it fixes the problem ?

It, the patch on the bug page, does (e.i. without the locking). Could I 
suggest a comment in the code.
> PS: and Raphael's patch while not fixing your bug is not technically 
> wrong as Windows checks for the surface description pointer being 
> non-NULL

Nor did I say that; just that it had nothing to with bug 3487 as the
subject said it had.

On Sun, 9 Oct 2005, Christian Costa wrote:
> This does not work if the destination size is smaller than the struct size or 
> source size is greater than destination size.
> Even worst, if source size is greater than struct size, 
> sizeof(*(to))-__fromsize is < 0.
> Why don't you just add the "if (to = from) break;" statement ?

I wrote the patch, not because of bug 3487, but because, as I
wrote in take 1:
> >

Pure luck to find this one. I didnt understand what was going on;
and rewrote it. I am still uncertain about invariants wrt. sizeof
and dwSize.
<<

It is impossible to follow. Ok; in the begining it was only a memcpy, then 
because the debug was gabled an memset was added. Then bug 2070 states the 
the (to)->dwSize could be too high. Rather than just adding a if 
statement, I decided that it needed a complete rewrite.

The invariants then is:
sizeof(*to) <= to->dwSize
sizeof(*from) == from->dwSize

Why is the old code testing the sizeof(*to) < to->dwsize for the memset 
case only? Why first memset and then copy over it? Why use 3 variables?

The goal was the do the memcpy and then memset the rest. The second goal 
was to bark if the invariants were broken. The third goal was to have the 
compiler remove any ifs when debugging was turned off.

I tried play around with

if(to != from) {
    ....
}

or

do {
    if (to!=from) {
    ...
    }
} while(0)

but found the

if (to!=from) break

cleanest.

The first ERR should have been a warn though.

Peter

[ddraw] Fix bug 3487 take 2

[Lionel Ulmer]

On Mon, Oct 10, 2005 at 01:43:04AM +0200, Peter Berg Larsen wrote:
> > Could you try it and tell if it fixes the problem ?
>
> It, the patch on the bug page, does (e.i. without the locking). Could I 
> suggest a comment in the code.

Well, you can suggest a comment or even send a patch adding comments if you
wish :-)

And as my patch is in CVS now, you can even do a regression test with the
'full blown' patch.
> Nor did I say that; just that it had nothing to with bug 3487 as the
> subject said it had.

Yeah, got confused too (it really took me a while to understand that we went
out of the Lock function before crashing).

Now back to your patch. The code looks like this:

/* __tosize can be set too large by some programs */
#define DD_STRUCT_COPY_BYSIZE(to,from)
do {
    DWORD __tosize = (to)->dwSize;
    DWORD __fromsize = (from)->dwSize;
    if ((to) == (from))
        break;
    if (__tosize > sizeof(*(to)))
        ERR("To struct's size too large");
    if (__fromsize > sizeof(*(from)))
        ERR("From struct's size too large");
    if (__fromsize > __tosize)
        ERR("Copying too large struct");
    memcpy(to,from,__fromsize);
    memset(to+__fromsize,0,sizeof(*(to))-__fromsize);    
    (to)->dwSize = __tosize;/*restore size*/
} while (0)

I would first merge both our patches replacing the following lines with an
'assert(to != from)':

if ((to) == (from))
        break;

This macro is only used to copy application data to Wine data or the
reverse. So if both pointers are the same, it is a Wine bug as we should
NEVER send any pointer to our private data to the application (they are not
const pointers => the application can modify them => all hell break loose).

After you also give a lot of warnings but you do not do as the previous
macro. Ie if 'fromsize' is bigger than 'tosize', you just print an error,
you do not 'correct' the size which would lead to a memory overflow.

Finally, 'to+__fromsize' is wrong, it should be '((char *) to)+__fromsize'
and why use 'sizeof(*(to))' as the basis for the 'memset' size and not
'tosize' ?

Lionel

[ddraw] Fix bug 3487 take 2

[Peter Berg Larsen]

On Mon, 10 Oct 2005, Lionel Ulmer wrote:
> > Nor did I say that; just that it had nothing to with bug 3487 as the
> > subject said it had.
>
> Yeah, got confused too (it really took me a while to understand that we went
> out of the Lock function before crashing).

Yep.
> /* __tosize can be set too large by some programs */
> #define DD_STRUCT_COPY_BYSIZE(to,from)
> do {
>    DWORD __tosize = (to)->dwSize;
>    DWORD __fromsize = (from)->dwSize;
>    if ((to) == (from))
>        break;
>    if (__tosize > sizeof(*(to)))
>        ERR("To struct's size too large");
>    if (__fromsize > sizeof(*(from)))
>        ERR("From struct's size too large");
>    if (__fromsize > __tosize)
>        ERR("Copying too large struct");
>    memcpy(to,from,__fromsize);
>    memset(to+__fromsize,0,sizeof(*(to))-__fromsize);
>    (to)->dwSize = __tosize;/*restore size*/
> } while (0)
> I would first merge both our patches replacing the following lines with an
> 'assert(to != from)':
>
> if ((to) == (from))
>        break;

I think thats execellent idea; I hadnt though of asserts, did not new 
wine used them..
> This macro is only used to copy application data to Wine data or the
> reverse. So if both pointers are the same, it is a Wine bug as we should
> NEVER send any pointer to our private data to the application (they are not
> const pointers => the application can modify them => all hell break loose).

Hmm, ok. I read the comments bug 2070, as we only did the wine data to 
application data. Otherwise I think bug 2070 is missing handling too large 
from dwSize and could corrupt mem by memcpy.
> After you also give a lot of warnings but you do not do as the previous
> macro. Ie if 'fromsize' is bigger than 'tosize', you just print an error,
> you do not 'correct' the size which would lead to a memory overflow.

Yes. The latter 2, if ERR, should be asserts too. The first if ERR should 
be a WARN, as some applications breaks this assumption.
> Finally, 'to+__fromsize' is wrong, it should be '((char *) to)+__fromsize'

yes.
> and why use 'sizeof(*(to))' as the basis for the 'memset' size and not
> 'tosize' ?

Because tosize is not to be trusted by bug 2070, it could be to large. One
of invariants was that tosize >= sizeof(*to).

I still havent got the grasp of what dwSize is for if it does not reflect 
the size allocated?, nor the size of the struct current in the mem.

Peter

[ddraw] Fix bug 3487 take 2

[Lionel Ulmer]

On Mon, Oct 10, 2005 at 10:18:21PM +0200, Peter Berg Larsen wrote:
> > I would first merge both our patches replacing the following lines with an
> > 'assert(to != from)':
> >
> > if ((to) == (from))
> >        break;
>
> I think thats execellent idea; I hadnt though of asserts, did not new 
> wine used them..

Well, as Alexandre committed my 'assert' patch, it's already in the code and
there is just a merge to be done :-)

As for asserts, they work because Wine catches the 'assert' signal and
transforms it into a Win32 exception thus starting the Wine debugger.
> I still havent got the grasp of what dwSize is for if it does not reflect 
> the size allocated?, nor the size of the struct current in the mem.

An example of the usage of the 'dwSize' parameter:

static HRESULT WINAPI
IDirectDrawSurface3Impl_Lock(LPDIRECTDRAWSURFACE3 This, LPRECT pRect,
                             LPDDSURFACEDESC pDDSD, DWORD dwFlags, HANDLE h)
{
    return IDirectDrawSurface7_Lock(CONVERT(This), pRect,
                                    (LPDDSURFACEDESC2)pDDSD, dwFlags, h);
}

As the DDSURFACEDESC2 structure starts exactly like the DDSURFACEDESC2 one,
the hack here is to use 'dwSize' to magically cast one structure to the
other to be able to share code.

This is what MS had in mind when they introduced this field: have
'extendable' structure wheer each new revisions only added fields => they
can all be filled by one common function, using the 'dwSize' field to
disctriminate between versions.

Note that next time I bring my Win32 laptop home, I will try to do some
tests in real Windows to see how Windows fills the given structure pointer.

Lionel

[ddraw] Fix bug 3487 take 2

[Peter Berg Larsen]

On Mon, 10 Oct 2005, Lionel Ulmer wrote:
> > I still havent got the grasp of what dwSize is for if it does not reflect
> > the size allocated?, nor the size of the struct current in the mem.
> As the DDSURFACEDESC2 structure starts exactly like the DDSURFACEDESC2 one,
> the hack here is to use 'dwSize' to magically cast one structure to the
> other to be able to share code.

Hmm, then it does reflect the size of the struct currently in the memory. 
So in theory it's:

assert(to != from);
DWORD __copysize = MIN((to)->dwSize,(from)->dwSize);
memcpy((to),(from),__copysize);

in practise, because of debug and bug 2070:

assert(to != from);
DWORD __tosize   = MIN(sizeof(*(to), ,(to)->dwSize);
DWORD __fromsize = MIN(sizeof(*(from),(from)->dwSize);
DWORD __copysize = MIN(__tosize,__fromsize);
memcpy((to),(from),__copysize);
memset((to)+__copysize,0,__tosize-__copysize);

Or am I still off?

Peter