Michael Karcher wrote:
Am Montag, den 13.10.2008, 21:05 -0400 schrieb Chris Ahrendt:
Fixed a trailing space error..
Sorry to chime in so late on this patchset.
First, some general remarks: The subject of your mail will be the one-line commit message. "Try #2" is not a good choice for that. Also it does not have enough context for the casual wine-patches reader, as the message overview does not show what you patch is about. The usual convention is to take the subject of the first try, and append (or prepend) "(try 2)" to that subject.
Subject: [PATCH] Pared Down Test For ddraw test... fixes error with IDirectDraw_CreateSurface where the test is not checking the return from the call. CreateSurface is returning a null.
Another convention is to *not* use the [PATCH] tag on wine-patches, as all mails on wine-patches are patches. Use "--keep-subject" with git-format-patch to prevent it. Last, but not least, your original subject was way too long. Subjects on wine-patches usually mention the component to be patched (that is usually the directory name without dlls or programs in front) followed by a short synopsis (around 40 to 60 characters, if possible). I suggest the following subject for your next try:
Subject: ddraw/tests: Catch failures of CreateSurface to prevent crash
I don't remember whether you had the crash on Windows or on Wine. You could put that info also into the subject. As it is more important what the goal of your patch is than how you achieve that, something like this might also be OK:
Subject: ddraw/tests: Don't crash on Windows with GeForce 4399, driver 42.23
(this subject using made-up numbers on purpose, please fit in your own, and only use this line if the crash *is* on native Windows)
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.
@@ -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. That doesnt make sense thought because the contents of desc has been set earlier. So unless the CreateSurface call nulls out the description parm before returning. Any ideas?
@@ -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.
So the only two variables used here are lpSurf and palette. These two variables are initialized at the point of definition.
| static void PaletteTest(void) | { | HRESULT hr; | LPDIRECTDRAWSURFACE lpSurf = NULL; | DDSURFACEDESC ddsd; | IDirectDrawPalette *palette = NULL;
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... 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. Its not that I don't know C (I have been writing in C for over 20 years so its not that.. (though I do admit at being rusty due to not doing anything but architecture and design)) . Any ideas?
BTW Thank you for your comments... I do appreciate them =) Chris
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
-
chris ahrendt -
Michael Karcher