August 2008 - Wine-Devel - list.winehq.org

Repeated messages (was Re: Adding Flawfinder to Patchwatcher)

by Erik de Castro Lopo

Austin English wrote: > I had a discussion with Dan about adding Flawfinder to the > patchwatcher. Is anyone else getting multiple copies of this message? I seem to be getting one every 10-20 minutes. A quick scan of the email headers suggests that this is getting resent from within the Gmail system. Each duplicate has a received line like: Received: by ey-out-1920.google.com with SMTP id 4so195432eyg.30 for <wine-devel(a)winehq.org>; Thu, 28 Aug 2008 20:50:50 -0700 (PDT) with a unique SMTP id. Erik -- ----------------------------------------------------------------- Erik de Castro Lopo ----------------------------------------------------------------- Beware the Lollipop of Mediocrity. Lick it once, and you suck forever.

Aug. 31, 2008

Re: Patch for font size = 0x7fff in dialog templates

by Koro

> This is a patch that fixes a discrepancy between WINE's dialog manager > and the real Windows one. If, in a dialog template, (assuming DS_SETFONT > is set) the font size is 0x7fff, it means that the dialog manager must > use the message box font and not read the rest of the font data > (facename and others) off the template. > > This quirk is "documented" in a comment there, and I have myself > confirmed this behavior with some of my code a few months ago: > http://blogs.msdn.com/oldnewthing/archive/2005/04/29/412577.aspx#413552 > > Sending patch as attachment because putting it inline would probably > word-wrap it all over the place. What was wrong with my patch? I never heard of it again, and last time I checked http://source.winehq.org/source/dlls/user32/dialog.c it still wasn't there.

Aug. 31, 2008

Re: Fix for rendering issues when calling GetDepthStencilSurface

by Rico Schüller

Andrew Fenn schrieb: > I'm really stumped on why this even works because after writing a test > (attached) and running on windows it shows that ppZStencilSurface IS > getting set to null when D3DERR_NOTFOUND is returned. > > Could it be that there is something else behind this problem which is > unrelated to what I have been looking at? > > Regards, > Andrew > > On Sat, Aug 30, 2008 at 1:05 AM, Stefan Dösinger <stefan(a)codeweavers.com> wrote: > >>> When IWineD3DDevice_GetDepthStencilSurface returns WINED3DERR_NOTFOUND >>> it was setting ppZStencilSurface to null. This was the cause of >>> rendering issues with the 3d models in spore. >>> >> Can you write a test for this? I think we have some depth stencil tests in dlls/d3d9/tests/device.c which you could use to test this. >> >> I think you are right with this patch - I vaguely remember writing a test for a different getter function, but it is better to be sure >> >> >> >> >> >> >> ------------------------------------------------------------------------ >> >> This is a better patch which shows the source of the problem and implements the correct behaviour of GetDepthStencilSurface. But there is a problem in function IWineD3DDeviceImpl_GetDepthStencilSurface. This couldn't return the value which we need (see test cases). So anyone a suggestion? Cheers Rico diff --git a/dlls/d3d9/device.c b/dlls/d3d9/device.c index ddeaca3..9a2aeb9 100644 --- a/dlls/d3d9/device.c +++ b/dlls/d3d9/device.c @@ -745,8 +745,14 @@ static HRESULT WINAPI IDirect3DDevice9Impl_GetDepthStencilSurface(LPDIRECT3DDE *ppZStencilSurface = NULL; } } else { - WARN("Call to IWineD3DDevice_GetDepthStencilSurface failed\n"); - *ppZStencilSurface = NULL; + WARN("Call to IWineD3DDevice_GetDepthStencilSurface failed with %i\n", hr); + if( hr == D3DERR_INVALIDCALL ) { + FIXME("D3DERR_INVALIDCALL %i\n", hr); + } + else if(hr == D3DERR_NOTFOUND) { + FIXME("D3DERR_NOTFOUND %i\n", hr); + *ppZStencilSurface = NULL; + } } LeaveCriticalSection(&d3d9_cs); return hr; diff --git a/dlls/d3d9/tests/device.c b/dlls/d3d9/tests/device.c index 41dcafb..b67ed93 100644 --- a/dlls/d3d9/tests/device.c +++ b/dlls/d3d9/tests/device.c @@ -1365,6 +1365,27 @@ static void test_depthstenciltest(void) ok(hr == D3DERR_NOTFOUND && pDepthStencil2 == NULL, "IDirect3DDevice9_GetDepthStencilSurface failed with %08x\n", hr); if(pDepthStencil2) IDirect3DSurface9_Release(pDepthStencil2); + /* This shows that pDepthStencil2 is set to 0 when hr == D3DERR_NOTFOUND */ +#define testnumber 10 + pDepthStencil2 = testnumber; + hr = IDirect3DDevice9_GetDepthStencilSurface(pDevice, &pDepthStencil2); + ok(hr == D3DERR_NOTFOUND && pDepthStencil2 == NULL, "IDirect3DDevice9_GetDepthStencilSurface failed %08x (%08x) pDepthStencil %08x (%08x)\n", hr, D3DERR_NOTFOUND, pDepthStencil2, 0); + if(pDepthStencil2 != testnumber && pDepthStencil2 != 0) IDirect3DSurface9_Release(pDepthStencil2); + + /* This shows that pDepthStencil2 is NOT set to 0 when hr == D3DERR_INVALIDCALL. + * Current implementation couldn't set it (see IWineD3DDeviceImpl_GetDepthStencilSurface) */ + + /* the DepthStencilSurface is NULL and Reset should add a new one ?? + * At least this provocates an INVALIDCALL in GetDepthStencil afterwards on windows, on wine (D3DERR_NOTFOUND)! */ + pDepthStencil2 = testnumber; /* set a value so that we could check if it is changed after an invalid call */ + hr = IDirect3DDevice9_Reset(pDevice, &d3dpp); + ok(hr == D3DERR_INVALIDCALL, "IDirect3DDevice9_Reset failed with %08x\n", hr); + hr = IDirect3DDevice9_GetDepthStencilSurface(pDevice, &pDepthStencil2); + ok(hr == D3DERR_INVALIDCALL && pDepthStencil2 == testnumber, "IDirect3DDevice9_GetDepthStencilSurface failed %08x (%08x) pDepthStencil %08x (%08x)\n", hr, D3DERR_INVALIDCALL, pDepthStencil2, testnumber); + if(pDepthStencil2 != testnumber && pDepthStencil2 != 0) IDirect3DSurface9_Release(pDepthStencil2); + pDepthStencil2 = 0; +exit(0); + /* This left the render states untouched! */ hr = IDirect3DDevice9_GetRenderState(pDevice, D3DRS_ZENABLE, &state); ok(hr == D3D_OK, "IDirect3DDevice9_GetRenderState failed with %08x\n", hr);

Aug. 31, 2008

Re: kernel32/winerror [try 3]: Add Russian translation

by Dmitry Timoshkov

"Vladimir Pankratov" <scriptkid(a)mail.ru> wrote: > Added Russian translation for winerrors. > > Added dlls/kernel32/nls/winerror_rus.mc file. > Changed files: > dlls/kernel32/kernel.rc > dlls/kernel32/Makefile.in What's the point in resending the patch without changing anything in it, or at least answering the questions? -- Dmitry.

Aug. 31, 2008

Re: Fix for rendering issues when calling GetDepthStencilSurface

by Rico Schüller

Andrew Fenn schrieb: > I'm really stumped on why this even works because after writing a test > (attached) and running on windows it shows that ppZStencilSurface IS > getting set to null when D3DERR_NOTFOUND is returned. > > Could it be that there is something else behind this problem which is > unrelated to what I have been looking at? > > Regards, > Andrew > > On Sat, Aug 30, 2008 at 1:05 AM, Stefan Dösinger <stefan(a)codeweavers.com> wrote: > >>> When IWineD3DDevice_GetDepthStencilSurface returns WINED3DERR_NOTFOUND >>> it was setting ppZStencilSurface to null. This was the cause of >>> rendering issues with the 3d models in spore. >>> >> Can you write a test for this? I think we have some depth stencil tests in dlls/d3d9/tests/device.c which you could use to test this. >> >> I think you are right with this patch - I vaguely remember writing a test for a different getter function, but it is better to be sure >> >> >> >> >> >> >> ------------------------------------------------------------------------ >> >> I've tried some ugly things and found a way to produce this in a test. The test works on windows but not on wine. Removing the *ppZStencilSurface = NULL; works for a part of the test. But the return value is different. Cheers Rico diff --git a/dlls/d3d9/tests/device.c b/dlls/d3d9/tests/device.c index 41dcafb..6ead34d 100644 --- a/dlls/d3d9/tests/device.c +++ b/dlls/d3d9/tests/device.c @@ -1365,6 +1365,18 @@ static void test_depthstenciltest(void) ok(hr == D3DERR_NOTFOUND && pDepthStencil2 == NULL, "IDirect3DDevice9_GetDepthStencilSurface failed with %08x\n", hr); if(pDepthStencil2) IDirect3DSurface9_Release(pDepthStencil2); + /* the DepthStencilSurface is NULL and Reset should add a new one ?? + * At least this provocates a INVALIDCALL in GetDepthStencil afterwards */ +#define testnumber 10 + pDepthStencil2 = testnumber; /* set a value so that we could check if it is changed after an invalid call */ + hr = IDirect3DDevice9_Reset(pDevice, &d3dpp); + ok(hr == D3DERR_INVALIDCALL, "IDirect3DDevice9_Reset failed with %08x\n", hr); + hr = IDirect3DDevice9_GetDepthStencilSurface(pDevice, &pDepthStencil2); + ok(hr == D3DERR_INVALIDCALL && pDepthStencil2 == testnumber, "IDirect3DDevice9_GetDepthStencilSurface failed %08x (%08x) pDepthStencil %08x (%08x)\n", hr, D3DERR_INVALIDCALL, pDepthStencil2, testnumber); + if(pDepthStencil2 != testnumber && pDepthStencil2 != 0) IDirect3DSurface9_Release(pDepthStencil2); + pDepthStencil2 = 0; +exit(0); + /* This left the render states untouched! */ hr = IDirect3DDevice9_GetRenderState(pDevice, D3DRS_ZENABLE, &state); ok(hr == D3D_OK, "IDirect3DDevice9_GetRenderState failed with %08x\n", hr);

Aug. 31, 2008

Re: Adding Flawfinder to Patchwatcher

by Rob Shearman

2008/8/28 Austin English <austinenglish(a)gmail.com>: > I had a discussion with Dan about adding Flawfinder to the > patchwatcher. Currently, it's got some pretty generic errors, but it > seems able to test only patches, so we wouldn't be flooded with old > nonbugs (or we could set up a blacklist of safe errors). For > reference, I've run it on today's git. I'm attaching the full log, as > well as a condensed version of the most common errors (1 per error > type). Looks like a lot of chances for buffer overflows.. > > Thoughts? Too many false positives to make it worth using. Just because you use strcpy, for example, it doesn't mean your program has a chance for a buffer overflow; it's using strcpy with a destination buffer that might not be large enough that causes buffer overflows. Having some kind of static analysis done on patches before Alexandre commits them is a good idea, but I don't think Flawfinder is the right static analysis tool to use. Coverity and Prefast are both static analysis tools with a bit more intelligence that identify bad code rather than just using "bad" functions. Other people may be able to suggest more good tools. -- Rob Shearman

Aug. 31, 2008

Re: Adding Flawfinder to Patchwatcher

by Reece Dunn

2008/8/28 Austin English <austinenglish(a)gmail.com>: > I had a discussion with Dan about adding Flawfinder to the > patchwatcher. Currently, it's got some pretty generic errors, but it > seems able to test only patches, so we wouldn't be flooded with old > nonbugs (or we could set up a blacklist of safe errors). For > reference, I've run it on today's git. I'm attaching the full log, as > well as a condensed version of the most common errors (1 per error > type). Looks like a lot of chances for buffer overflows.. > > Thoughts? +1 This looks good, but there does seem to be a large amount of noise and it seems to generate warnings without being able to identify correct usage. These issues will need to be verified (i.e. the NULL DACLs used in the tests and the potential buffer overflows). It would be interesting to see what results sparse and smatch generate, and if they (or valgrind) can be extended to identify these (with the possibility of ignoring them on correct usage) and more. This could also be extended to the resources: checking that there are no duplicate mnemonics, that controls that need a label have one and other usability issues. - Reece

Aug. 31, 2008

Re: [msi/tests 3/3] Create a system restore point when needed

by Paul Vriens

James Hawkins wrote: > On Sat, Aug 30, 2008 at 3:14 PM, Paul Vriens <paul.vriens.wine(a)gmail.com> wrote: >> Hi, >> >> The slowness on some machines for the install tests comes from the system >> restore facility. When 'System Restore' is active you will find a great >> number >> of system restore points after these tests have run. Adding our own system >> restore point circumvents this as Windows will now wait till we tell it that >> the >> system change has ended. Only 1 system restore point will be created now >> and >> this one is nicely removed afterwards. >> >> On my testboxes (XP and Vista) this makes sure that the tests run within the >> limits of the timeout (2 minutes). Before this patch the tests would take >> about >> 8 minutes on my Vista box (now 1.something). >> >> Changelog >> Create a system restore point when needed >> > > Please factor all of this out into functions. I'll give that a shot. > > + /* Create a system restore point. We need to do this on a system > where 'System > + * Restore' is active as we would otherwise timeout on these tests. > + * By doing so we circumvent the multitude of restore points that > would have been > + * created by all the installation and removal tests. > + * > + * The restore point is removed afterwards. > + * > + * On systems where 'System Restore' is not active, this won't > have any effect > + * as the first call to SRSetRestorePointA will fail with a status of > + * ERROR_SERVICE_DISABLED. > + */ > > Does the comment really need to be that long? > I think I can reduce that to a 2-liner or so. During testing I added stuff and stuff that's why it's maybe too much. -- Cheers, Paul.

Aug. 31, 2008

Re: Recent msi/package tests failures

by Paul Vriens

James Hawkins wrote: > On Sat, Aug 30, 2008 at 3:17 PM, Paul Vriens <paul.vriens.wine(a)gmail.com> wrote: >> Hi, >> >> I just sent a few patches that fix the problem with the timeout on my >> machines. >> >> Do you think it's still worthwhile to disable logging by using >> MsiEnableLogA? When I now look in my temp folder I have a few hundred MSI >> log files. We could either disable logging or create 1 file ourselves and >> append everything to there (removing it afterwards maybe). >> > > Logging to one file takes just as long as any other type of logging. > No logging should be happening. No I don't think it's worth using > MsiEnableLog to disable loggin. Logging is not enabled by default, > and to enable it you have to change some registry entries, which I > doubt anyone is doing. > The reason for the 1 file was not to speed up things but to limit the number of logfiles in the temp directory (and maybe the ability to remove this file as we know the one we created ourselves). On all of my boxes (95/98/NT/W2K/ 2 times XP/W2K3/Vista) I end up with 200 or so logfiles in the temp directory after running the install tests. On none of these boxes did I do anything to tinker with the msi logging settings. It would be nice if other could tell if they have a huge number of these logfiles in their temp directory after running the tests. -- Cheers, Paul.

Aug. 31, 2008

Re: ntdll: add some tests for NtDeleteFile

by Jeff Zaroyko

On Sun, Aug 31, 2008 at 3:55 AM, Vitaliy Margolen <wine-devel(a)kievinfo.com> wrote: > Jeff Zaroyko wrote: >> >> >> + static const WCHAR testdirW[] = {'n','t','d','e','l','e','t','e','f','i','l','e',0}; >> + static const WCHAR subdirW[] = {'\\','s','u','b',0}; >> + >> + ret = CreateDirectoryW(testdirW, NULL); > You need to create test files and directories in windows temp dir. ok, I've sent a new patch that uses the temp dir. > >> + pRtlDosPathNameToNtPathName_U(testdirW, &nameW, NULL, NULL); > > You are leaking unicode string nameW. You need to call this to free it: > RtlFreeUnicodeString( &nameW ); Thanks >> + ret = pNtDeleteFile(&attr); >> + ok(ret == STATUS_SUCCESS, "expected NtDeleteFile to ret STATUS_SUCCESS on failure\n"); > How do you know it's a failure? You should check if dir is still there. I check the return value from RemoveDirectory to test if it was still there. I think the revised patch is clearer, I'll fix my kernel32 tests patch too. > Vitaliy >

Aug. 31, 2008