Try #2

[Michael Karcher]

To all: sorry for so much quoting, but I don't think trimming the quotes
down helps understanding my point. If you are annoyed, we can move the
discussion off-list.

Am Dienstag, den 14.10.2008, 09:16 -0700 schrieb chris ahrendt:   
> >> diff --git a/dlls/ddraw/tests/dsurface.c b/dlls/ddraw/tests/dsurface.c
> >> index fc7bb88..21e6b15 100644
> >> --- a/dlls/ddraw/tests/dsurface.c
> >> +++ b/dlls/ddraw/tests/dsurface.c
> >> @@ -1181,7 +1181,10 @@ static void AttachmentTest7(void)
> >>      ddsd.dwHeight = GetSystemMetrics(SM_CYSCREEN);
> >>      hr = IDirectDraw7_CreateSurface(dd7, &ddsd, &surface3, NULL);
> >>      ok(hr==DD_OK,"CreateSurface returned: %x\n",hr);
> >> -
> >> +    if (FAILED(hr)) {
> >> +       skip("failed to create surface3\n");
> >> +       return;
> >> +    }
> >>      /* This one has a different size */
> >>      memset(&ddsd, 0, sizeof(ddsd));
> >>      ddsd.dwSize = sizeof(ddsd);
> >>     
> > return inside a test function is bad style. You may use it for try&crash
> > debugging, but I don't see any chance to get such a patch committed. In
> > this case, you leak one IDirectDraw7 interface, forget to restore the
> > cooperative level and you leak two surfaces. The "goto err:" approach is
> > preferred if a lot of cleanup is needed (like here). You either want to
> > make a label "err2:" before the IDirectDrawSurface7_Release(surface2)
> > and use that label (analogous for "err3:" one line before that) or use
> > the technique of setting released or uninitialized interface pointers to
> > NULL and NULL-Check before calling release.
> >
> > Same remarks for the next 3 hunks (not quoted).
> >
> Thing is the two surfaces have not been allocated yet.. they are off
> the local stack and just set to null. 
That's why I suggested you to introduce the err2 label. The end of the
function should look like this:

dlls/ddraw/tests/dsurface.c:1206
|     IDirectDrawSurface7_Release(surface4);
> err3:
|     IDirectDrawSurface7_Release(surface3);
> err2:
|     IDirectDrawSurface7_Release(surface2);
|     IDirectDrawSurface7_Release(surface1);
| 
|     hr =IDirectDraw7_SetCooperativeLevel(dd7, NULL, DDSCL_NORMAL);
|     ok(hr == DD_OK, "SetCooperativeLevel returned %08x\n", hr);
|     IDirectDraw7_Release(dd7);
| }

If in the error case you goto err2, the two surfaces that don't have
been allocated yet don't get released, as you skip the Release calls for
surface4 and surface3. (Thats why I called this label "err2": It only
deallocates two surfaces)

> >> @@ -2208,6 +2220,12 @@ static void SizeTest(void)
> >>      desc.dwWidth = 128; /* Keep them set to  check what happens */
> >>      ret = IDirectDraw_CreateSurface(lpDD, &desc, &dsurface, NULL);
> >>      ok(ret == DD_OK, "Creating a primary surface without width and height info returned %08x\n", ret);
> >> +    /* This should be handled but CreateSurface is failing */
> >> +    if (FAILED(ret))
> >> +    {
> >> +        skip("Can't create cubemap surface\n");
> >> +        return;
> >> +    }
> >>      if(dsurface)
> >>      {
> >>          ret = IDirectDrawSurface_GetSurfaceDesc(dsurface, &desc);
> >>     
> > The skip message is wrong. It's not about cubemap at all. Also, this
> > hunk is unneeded. If CreateSurface fails, dsurface is unmodified or set
> > to NULL (or CreateSurface really badly needs fixing!). As dsurface is
> > NULL on the call to CreateSurface, it is null if CreateSurface failed.
> > So the condition of the if statement following the CreateSurface call is
> > false, and no crash can happen on that NULL pointer.

> Yes I just caught this message thanks .... I will fix that... The 
> problem here which makes no sense to me is if I don't have the
> Failed statement before the dsurface if when it gets the surface 
> description it throws an exception because one of the parms is null.
Which parameter do you mean? The first parameter is dsurface. dsurface
can't be NULL, as it has been tested to not be NULL two lines before the
GetSurfaceDesc call. The other parameter is "&desc". Please note the
ampersand in front of desc, which means address-of. As desc is a
structure allocated on the stack, its address is also in no case NULL,
so GetSurfaceDesc can't crash.

> That doesnt make sense thought because the contents of desc has been set 
> earlier.
Yeah, right. The contents of desc have been set before, but
GetSurfaceDesc does not care except for the dwSize member. All other
members of desc will be overwritten by GetSurfaceDesc.

> So unless the CreateSurface call nulls out the
> description parm before returning. Any ideas?
CreateSurface does not modify the desc parameter. But how does it
matter?

> >> @@ -2492,7 +2510,7 @@ static void PaletteTest(void)
> >>      ok(hr==DD_OK, "CreateSurface returned: %x\n",hr);
> >>      if (FAILED(hr)) {
> >>     skip("failed to create surface\n");
> >> -   goto err;
> >> +   return;
> >>      }
> >>  
> >>      hr = IDirectDrawSurface_SetPalette(lpSurf, palette);
> >>     
> > Why do you need this hunk? You talked about usage of uninitialized
> > variables at the target of the goto, but that's not the case. The code
> > at err looks like this (formatting adjusted for clearness):
> >
> > | err:
> > |     if (lpSurf) IDirectDrawSurface_Release(lpSurf);
> > |     if (palette) IDirectDrawPalette_Release(palette);
> >
> >   
> Again when I leave the goto err in and it does the release it throws an 
> exception on the lpsurf line.
Very strange. May I repeat the question on what platform with what
graphics card that happens? The lpSurf line *should* not throw an
exception if lpSurf really is NULL, as it does nothing in this case, so
the only case it can throw is that CreateSurface failed and still
returned a non-null pointer in lpSurf. This is never supposed to happen,
but if you can show that it happens on your computer (for example,
trace() hr and lpSurf at that point), you need to work around this
serious implementation flaw. While your return helps in that case, you
still leak the palette. The correct fix would be to explicitly set
lpSurf to NULL before jumping to err.

> > So the only two variables used here are lpSurf and palette. These two
> > variables are initialized at the point of definition.
> >
> > As they are initialized to NULL, the condition of the if statement is
> > false if these variables were not yet initialized. So no crash possible
> > here. Your return statement leaks palette.
> Yes I saw that.. which is why it did not make much sense...
Sorry, I don't understand *what* did not make much sense. Could you try
to be a bit clearer in your messages?

> if you want 
> I can recreate and show you the exception which occurs when I don't have 
> the return or the dsurface error I mentioned above.
That would be interesting. But it is enough if you can quote the values
of hr and lpSurf in the error case (put them in the skip message!). If
you really do get a hr value indicating an error (highest bit set) and
lpSurf is not NULL, some action is required, but up to you showing the
output, I keep very sceptical that this happens on any real-world
system. If you post the output of a modified test (as I suggested),
*please* also post the code you used to generate that output.

Regards,
  Michael Karcher