May 2011 - Wine-Devel - list.winehq.org

Re: [2/2] comctl32/mru: Ignore size field when creating with -W calls

by Alexandre Julliard

Nikolay Sivov <nsivov(a)codeweavers.com> writes: > @@ -210,11 +210,11 @@ typedef INT (CALLBACK *MRUStringCmpFnA)(LPCSTR lhs, LPCSTR rhs); > typedef INT (CALLBACK *MRUStringCmpFnW)(LPCWSTR lhs, LPCWSTR rhs); > typedef INT (CALLBACK *MRUBinaryCmpFn)(LPCVOID lhs, LPCVOID rhs, DWORD length); > > -typedef struct tagCREATEMRULISTA > +typedef struct tagMRUINFOA > { > DWORD cbSize; > - DWORD nMaxItems; > - DWORD dwFlags; > + UINT uMax; > + UINT fFlags; > HKEY hKey; > LPSTR lpszSubKey; > union Please do the renamings as a separate patch. -- Alexandre Julliard julliard(a)winehq.org

May 2, 2011

Re: [1/5] cmd: WCMD_delete: simplify logic, avoid assigning variable to itself

by Henri Verbeet

On 2 May 2011 02:23, Dan Kegel <dank(a)kegel.com> wrote: > - if (attribute && !negate) { > - stillOK = stillOK; > - } else if (!attribute && negate) { > - stillOK = stillOK; > - } else { > + if (attribute == negate) > stillOK = FALSE; > - } If you just write that as "if (!attribute == !negate)" you avoid both the assumption that TRUE is defined to 1 and the ugly "!= 0"'s. Or you could build actual attribute masks first and compare those instead, which may even be readable.

May 2, 2011

Re: [PATCH 1/5] mmdevapi: Add InitializeDriver() method to drivers

by Alexandre Julliard

Andrew Eikum <aeikum(a)codeweavers.com> writes: > @@ -72,11 +73,19 @@ static BOOL load_driver(const WCHAR *name) > } > > #define LDFC(n) do { drvs.p##n = (void*)GetProcAddress(drvs.module, #n);\ > - if(!drvs.p##n) return FALSE; } while(0); > + if(!drvs.p##n) { FreeLibrary(drvs.module); return FALSE; } } while(0); > + LDFC(InitializeDriver); > LDFC(GetEndpointIDs); > LDFC(GetAudioEndpoint); > #undef LDFC > > + hr = drvs.pInitializeDriver(); > + if(FAILED(hr)){ > + TRACE("Driver initialization failed: %08x\n", hr); > + FreeLibrary(drvs.module); > + return hr; > + } If you are going to call it on load, you could just as well do the init in the driver on DLL_PROCESS_ATTACH and avoid the extra method. -- Alexandre Julliard julliard(a)winehq.org

May 2, 2011

Re: comctl32: realloc or free and alloc

by Alexey Fisher

On So, 2011-05-01 at 21:55 +0400, Nikolay Sivov wrote: > On 5/1/2011 21:49, Alexey Fisher wrote: > > On So, 2011-05-01 at 19:11 +0400, Nikolay Sivov wrote: > >> On 5/1/2011 13:07, Alexey Fisher wrote: > >>> There is a new patch in the attachment. > >>> BOOL Str_SetPtrAtoW (LPWSTR *lppDest, LPCSTR lpSrc) > >>> { > >>> - TRACE("(%p %s)\n", lppDest, lpSrc); > >>> + TRACE("(%p, %s)\n", *lppDest, debugstr_a(lpSrc)); > >>> + > >>> + if (*lppDest) { > >>> + ERR("lppDest should be NULL!"); > >>> + return FALSE; > >>> + } > >> It's an internal call, so it's better to require a caller to pass valid > >> parameters. That's why ERR is too much here, cause you completely > >> control passed parameters in a first place. > > The problem is, this function return FALSE if some thing going wrong, > > but caller never check it. So if we pass fresh not NULL, not Alloc'd > > pointer. We have some garbage in pointers target before the call. After > > ReAlloc and Str_SetPtrAtoW failed, caller continues to use old pointers > > target, also old garbage. The App get garbage instead of the string and > > will crash in some conditions. This all make it hard to find the cause > > of the crash. > I'll try to explain again. > > Str_SetPtrAtoW() is not a public API call, so it's not accessible directly from outside > comctl32. My request to you was to provide a simple test application that uses some comctl32 controls > presumably and indirectly makes use of this call (and fails for some reason you'll trying to find). > > >> Please open a bug with a test C application so we can look at initial > >> problem. > > Das Not ReAllocing not Alloc'd memory is bug? or it is future? > It depends. > > Here is part of man realloc: > > > >> realloc() changes the size of the memory block pointed to by ptr to > >> size bytes. The contents will be unchanged to the minimum of the old > >> and new sizes; newly allocated memory will be uninitialized. If ptr is > >> NULL, then the call is equivalent to malloc(size), for all values of > >> size; if size is equal to zero, and ptr is not NULL, then the call is > >> equivalent to free(ptr). Unless ptr is NULL, it must have been > >> returned by an earlier call to malloc(), calloc() or realloc(). If the > >> area pointed to was moved, a free(ptr) is done. > > This is irrelevant here. I need to apologize. Wine a big mystery for me. And it seems like the string corruption i talked about, i introduced by my self. And i see you send the patch to test the strings: comctl32/tests: Cleanup MRU lists tests, add more At least i got the target and found how to workaround my bug http://bugs.winehq.org/show_bug.cgi?id=26923 :D -- Regards, Alexey

May 2, 2011

Re: comctl32: realloc or free and alloc

by Nikolay Sivov

On 5/1/2011 21:49, Alexey Fisher wrote: > On So, 2011-05-01 at 19:11 +0400, Nikolay Sivov wrote: >> On 5/1/2011 13:07, Alexey Fisher wrote: >>> There is a new patch in the attachment. >>> BOOL Str_SetPtrAtoW (LPWSTR *lppDest, LPCSTR lpSrc) >>> { >>> - TRACE("(%p %s)\n", lppDest, lpSrc); >>> + TRACE("(%p, %s)\n", *lppDest, debugstr_a(lpSrc)); >>> + >>> + if (*lppDest) { >>> + ERR("lppDest should be NULL!"); >>> + return FALSE; >>> + } >> It's an internal call, so it's better to require a caller to pass valid >> parameters. That's why ERR is too much here, cause you completely >> control passed parameters in a first place. > The problem is, this function return FALSE if some thing going wrong, > but caller never check it. So if we pass fresh not NULL, not Alloc'd > pointer. We have some garbage in pointers target before the call. After > ReAlloc and Str_SetPtrAtoW failed, caller continues to use old pointers > target, also old garbage. The App get garbage instead of the string and > will crash in some conditions. This all make it hard to find the cause > of the crash. I'll try to explain again. Str_SetPtrAtoW() is not a public API call, so it's not accessible directly from outside comctl32. My request to you was to provide a simple test application that uses some comctl32 controls presumably and indirectly makes use of this call (and fails for some reason you'll trying to find). >> Please open a bug with a test C application so we can look at initial >> problem. > Das Not ReAllocing not Alloc'd memory is bug? or it is future? It depends. > Here is part of man realloc: > >> realloc() changes the size of the memory block pointed to by ptr to >> size bytes. The contents will be unchanged to the minimum of the old >> and new sizes; newly allocated memory will be uninitialized. If ptr is >> NULL, then the call is equivalent to malloc(size), for all values of >> size; if size is equal to zero, and ptr is not NULL, then the call is >> equivalent to free(ptr). Unless ptr is NULL, it must have been >> returned by an earlier call to malloc(), calloc() or realloc(). If the >> area pointed to was moved, a free(ptr) is done. > This is irrelevant here.

May 1, 2011

Re: comctl32: realloc or free and alloc

by Alexey Fisher

On So, 2011-05-01 at 19:11 +0400, Nikolay Sivov wrote: > On 5/1/2011 13:07, Alexey Fisher wrote: > > > > There is a new patch in the attachment. > > > BOOL Str_SetPtrAtoW (LPWSTR *lppDest, LPCSTR lpSrc) > > { > > - TRACE("(%p %s)\n", lppDest, lpSrc); > > + TRACE("(%p, %s)\n", *lppDest, debugstr_a(lpSrc)); > > + > > + if (*lppDest) { > > + ERR("lppDest should be NULL!"); > > + return FALSE; > > + } > It's an internal call, so it's better to require a caller to pass valid > parameters. That's why ERR is too much here, cause you completely > control passed parameters in a first place. The problem is, this function return FALSE if some thing going wrong, but caller never check it. So if we pass fresh not NULL, not Alloc'd pointer. We have some garbage in pointers target before the call. After ReAlloc and Str_SetPtrAtoW failed, caller continues to use old pointers target, also old garbage. The App get garbage instead of the string and will crash in some conditions. This all make it hard to find the cause of the crash. > Please open a bug with a test C application so we can look at initial > problem. Das Not ReAllocing not Alloc'd memory is bug? or it is future? Here is part of man realloc: > realloc() changes the size of the memory block pointed to by ptr to > size bytes. The contents will be unchanged to the minimum of the old > and new sizes; newly allocated memory will be uninitialized. If ptr is > NULL, then the call is equivalent to malloc(size), for all values of > size; if size is equal to zero, and ptr is not NULL, then the call is > equivalent to free(ptr). Unless ptr is NULL, it must have been > returned by an earlier call to malloc(), calloc() or realloc(). If the > area pointed to was moved, a free(ptr) is done. -- Regards, Alexey

May 1, 2011

Re: comctl32: realloc or free and alloc

by Vitaliy Margolen

On 05/01/2011 10:29 AM, Alexey Fisher wrote: > Correct me if I'm wrong. You can't ReAlloc pointer only if you allocated > the memory by Alloc. At least for glibc. First of all we talking about Wine's functions HeapAlloc, HeapReAlloc, and HeapFree. The HeapReAlloc can and does re-allocate memory initially allocated by HeapAlloc. However it can not allocate memory first time around - calling HeapReAlloc with null src pointer is an error. What Dan was talking about - HeapReAlloc will attempt to reuse the same memory if new size is same or smaller then current size. This can significantly improve performance. Vitaliy.

May 1, 2011

Re: comctl32: realloc or free and alloc

by Alexey Fisher

On So, 2011-05-01 at 08:05 -0700, Dan Kegel wrote: > On Sun, May 1, 2011 at 2:07 AM, Alexey Fisher > <bug-track(a)fisher-privat.net> wrote: > >> > The code looks like it assumes that pointer is always > >> > managed by Alloc/ReAlloc/Free. In what context is > >> > the destination a fresh, non-Alloc'd pointer? > >> > Perhaps that's where the bug lies. > >> > >> We do not need to save anything what was in the target. Even the old > >> code ReAloocate it and then rewrite. In most contexts the pointer is a > >> fresh one. If it is not, it should be freed before this func. > > I don't see why we should need to change the contract > so much. Re-using existing blocks of memory isn't > the worst idea in the world. Probably not, but: Correct me if I'm wrong. You can't ReAlloc pointer only if you allocated the memory by Alloc. At least for glibc. > > There is a new patch in the attachment. > > It seems there's still some mystery in the code, > and that you don't fully understand it, so your > patch probably isn't right. you right. I'll continue to digg and send new patch ASAP. -- Regards, Alexey

May 1, 2011

Re: comctl32: realloc or free and alloc

by Nikolay Sivov

On 5/1/2011 13:07, Alexey Fisher wrote: > > There is a new patch in the attachment. > BOOL Str_SetPtrAtoW (LPWSTR *lppDest, LPCSTR lpSrc) > { > - TRACE("(%p %s)\n", lppDest, lpSrc); > + TRACE("(%p, %s)\n", *lppDest, debugstr_a(lpSrc)); > + > + if (*lppDest) { > + ERR("lppDest should be NULL!"); > + return FALSE; > + } It's an internal call, so it's better to require a caller to pass valid parameters. That's why ERR is too much here, cause you completely control passed parameters in a first place. Please open a bug with a test C application so we can look at initial problem.

May 1, 2011

Re: comctl32: realloc or free and alloc

by Dan Kegel

On Sun, May 1, 2011 at 2:07 AM, Alexey Fisher <bug-track(a)fisher-privat.net> wrote: >> > The code looks like it assumes that pointer is always >> > managed by Alloc/ReAlloc/Free. In what context is >> > the destination a fresh, non-Alloc'd pointer? >> > Perhaps that's where the bug lies. >> >> We do not need to save anything what was in the target. Even the old >> code ReAloocate it and then rewrite. In most contexts the pointer is a >> fresh one. If it is not, it should be freed before this func. I don't see why we should need to change the contract so much. Re-using existing blocks of memory isn't the worst idea in the world. > There is a new patch in the attachment. It seems there's still some mystery in the code, and that you don't fully understand it, so your patch probably isn't right. - Dan

May 1, 2011