October 2012 - Wine-Devel - list.winehq.org

Re: crypt32: Avoid double free in CRYPT_LoadSIP on error path (coverity))

by Nikolay Sivov

On 10/29/2012 11:33, Frédéric Delanoy wrote: > FreeLibrary(temp) can be called twice on error. > > CID 714004 > --- > > error: > FreeLibrary(lib); > - FreeLibrary(temp); > + if (temp != NULL) FreeLibrary(temp); > SetLastError(TRUST_E_SUBJECT_FORM_UNKNOWN); > return FALSE; > } This is useless, FreeLibrary() checks for NULL.

Oct. 29, 2012

Re: [PATCH] Potential reference count races

by Jacek Caban

On 10/29/12 01:25, Max TenEyck Woodbury wrote: > On 10/28/2012 12:06 PM, Nikolay Sivov wrote: >> On 10/28/2012 17:44, Max TenEyck Woodbury wrote: >>> On 10/28/2012 02:40 AM, Nikolay Sivov wrote: >>>> On 10/28/2012 04:59, max(a)mtew.isa-geek.net wrote: >>>>> From: Max TenEyck Woodbury <max(a)mtew.isa-geek.net> >>>>> >>>>> I have been looking at the Microsoft COM and related documentations >>>>> and noticed that they emphatically recommend using the Interlocked... >>>>> functions when manipulating reference counts. I managed to set up a >>>>> search that showed where many of the reference count updates occur and >>>>> was somewhat surprised at how often this advice was not followed. >>>> It doesn't mean it always has to be followed. >>> True in a limited sense, but there is a good reason behind their >>> recommendation. Unless there is a good reason not to do so, this >>> particular piece of advice should be followed. >> COM objects in wine follow this recommendation in general, even object >> itself is not thread safe. >> This doesn't mean however that you need this every time you have some >> kind of refcount of any sort. > It may or may not be necessary every time, but it should be demonstrated > that it is not necessary rather than assumed that it is not. This is a > 'race condition' after all, and they are known to be rare and difficult > to isolate. I think it is good practice to assume there could be a race > problem rather than otherwise. No, it's a good practice to understand what you're changing instead of blindly assuming that everything with 'ref' in its name needs Interlocked* functions. You're patch even changes things in jscript that are not COM objecst, in a middle of code that is not thread safe anyway. It's documented that those parts of the code are not thread safe. Even worse, you were changing lines that even have a comment about no need for atomic operations (thanks for Michael, who predicted that someone may send patches like you). >>>>> While I have not converted every reference count update to use the >>>>> Interlocked... functions, this set of patches fixes a fair number of >>>>> them. >>>>> >>>>> These are not associated with any particular bug report; they are >>>>> simply a general precaution against operations that are known to be >>>>> associated with race conditions. >>>> This precaution doesn't work in general. It's not enough to atomically >>>> update refcount to make an implementation thread safe. Also not >>>> everything is supposed to be thread safe in a first place. >>>> >>> First, explain what does not have to be thread safe. >> Anything really, COM objects in particular if you were talking about them. >> > I think you are talking about the apartment model here, which forces > thread serialization. Despite that, the Microsoft documentation still > recommends interlocked operations on reference counts... Just to make it clear: using Interlocked* functions in generic COM object is a good practice. But lack of it is not always a bug and you shouldn't change it without understanding what you are doing. >>> I believe that application may try to use multiple threads anywhere, >>> so everything that can be made thread safe, should be. >>> >> No. > What do you mean 'No'. That is an opinion, If you disagree, please > explain why. See above. >>>> Changes like this: >>>> >>>>> - for (i=0;i<howmuch;i++) >>>>> + for (i=0;i<howmuch;++i) >>>>> TRACE("notify at %d to %p\n", >>>>> notify[i].dwOffset,notify[i].hEventNotify); >>>> are not helpful at all. >>>> >>> The post increment and decrement operation are specified as saving >>> the original value for use in the evaluation of the expression they >>> are part of and modifying the underlying stored value. In expressions >>> like this, that saved value is then discarded. The optimization phase >>> of the compilation usually removes both the save and discard operations. >> Sure, but I don't think it's enough to justify such changes all over the >> place, in existing code. >> > I agree that it is not enough to justify a separate set of patches, but > as part of another set of changes, I think it is justified. After all, > how else are examples of bad code going to be removed. This is not a bad coding. You're changing a lot of my code, which uses my coding style and this style is to use postfix incrementation. I'm not saying it's better of worse because it's not, so there is no reason to change it. Your argument about requirement for temporary storage is an example of bad way to think about the code. Jacek

Oct. 29, 2012

Re: dplayx: Reorder some code to avoid memory leaks (coverity)

by Rico Schüller

On 28.10.2012 16:13, André Hentschel wrote: > - lpGData->lpRemoteData = lpNewData; > + lpGData->lpRemoteData = HeapAlloc( GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof( dwDataSize ) ); > + CopyMemory( lpGData->lpRemoteData, lpData, dwDataSize ); > lpGData->dwRemoteDataSize = dwDataSize; > - lpPData->lpRemoteData = lpNewData; > + lpPData->lpRemoteData = HeapAlloc( GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof( dwDataSize ) ); > + CopyMemory( lpPData->lpRemoteData, lpData, dwDataSize ); > lpPData->dwRemoteDataSize = dwDataSize; Is the HEAP_ZERO_MEMORY really needed? You may kill that too while you change those lines. Cheers Rico

Oct. 29, 2012

Re: [PATCH] kernel32: Implement CompareStringOrdinal.

by Nikolay Sivov

On 10/29/2012 00:53, Christian Costa wrote: > --- > dlls/kernel32/kernel32.spec | 1 + > dlls/kernel32/locale.c | 8 ++++++++ > 2 files changed, 9 insertions(+) > > diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec > index b7efa0f..0bd1adc 100644 > --- a/dlls/kernel32/kernel32.spec > +++ b/dlls/kernel32/kernel32.spec > @@ -201,6 +201,7 @@ > @ stdcall CompareStringA(long long str long str long) > @ stdcall CompareStringW(long long wstr long wstr long) > @ stdcall CompareStringEx(wstr long wstr long wstr long ptr ptr long) > +@ stdcall CompareStringOrdinal(wstr long wstr long long) > @ stdcall ConnectNamedPipe(long ptr) > @ stub ConsoleMenuControl > @ stub ConsoleSubst > diff --git a/dlls/kernel32/locale.c b/dlls/kernel32/locale.c > index c41442c..83de815 100644 > --- a/dlls/kernel32/locale.c > +++ b/dlls/kernel32/locale.c > @@ -3047,6 +3047,14 @@ INT WINAPI CompareStringA(LCID lcid, DWORD flags, > return ret; > } > > +/****************************************************************************** > + * CompareStringOrdinal (KERNEL32.@) > + */ > +INT WINAPI CompareStringOrdinal(LPCWSTR str1, INT len1, LPCWSTR str2, INT len2, BOOL ignore_case) > +{ > + return CompareStringEx(NULL, ignore_case ? NORM_IGNORECASE : 0, str1, len1, str2, len2, NULL, NULL, 0); > +} > + > /************************************************************************* > * lstrcmp (KERNEL32.@) > * lstrcmpA (KERNEL32.@) This is not a way to do it, it doesn't simply work as that. See http://msdn.microsoft.com/en-us/library/windows/desktop/dd318144%28v=vs.85%… And it need tests of course.

Oct. 29, 2012

Re: [PATCH] Remove potential reference count races

by Max TenEyck Woodbury

On 10/28/2012 12:07 PM, Nikolay Sivov wrote: > On 10/28/2012 18:00, Max TenEyck Woodbury wrote: >> On 10/28/2012 03:13 AM, Nikolay Sivov wrote: >>> On 10/28/2012 04:59, max(a)mtew.isa-geek.net wrote: >>>> From: Max TenEyck Woodbury <max(a)mtew.isa-geek.net> >>>> >>>> --- >>>> dlls/shlwapi/thread.c | 4 ++-- >>>> 1 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/dlls/shlwapi/thread.c b/dlls/shlwapi/thread.c >>>> index eb2c35d..43e0433 100644 >>>> --- a/dlls/shlwapi/thread.c >>>> +++ b/dlls/shlwapi/thread.c >>>> @@ -157,7 +157,7 @@ static ULONG WINAPI threadref_AddRef(IUnknown >>>> *iface) >>>> threadref * This = impl_from_IUnknown(iface); >>>> TRACE("(%p)\n", This); >>>> - return InterlockedIncrement(This->ref); >>>> + return InterlockedIncrement(&This->ref); >>>> } >>>> static ULONG WINAPI threadref_Release(IUnknown *iface) >>>> @@ -167,7 +167,7 @@ static ULONG WINAPI threadref_Release(IUnknown >>>> *iface) >>>> TRACE("(%p)\n", This); >>>> - refcount = InterlockedDecrement(This->ref); >>>> + refcount = InterlockedDecrement(&This->ref); >>>> if (!refcount) >>>> HeapFree(GetProcessHeap(), 0, This); >>>> >>> Did you try to build this? >>> >>> >> Yes, it built. I don't submit stuff that causes the compiler to barf >> unless I am completely totally zonked from lack of sleep and this was >> not what happened here. I will admit that I have not looked at the >> build and test logs closely, so there may be problems I missed. >> > These changes I wrong that's what I'm saying. You could well be right about this one. All the other '->ref's are LONG or ULONG or DWORD and the '&' is necessary. If this one is a pointer, then I messed up. > > P.S. please CC wine-devel list in your replies. > Pushed the wrong button when setting up for the reply. Apologies.

Oct. 29, 2012

Re: [PATCH] Potential reference count races

by Max TenEyck Woodbury

On 10/28/2012 12:06 PM, Nikolay Sivov wrote: > On 10/28/2012 17:44, Max TenEyck Woodbury wrote: >> On 10/28/2012 02:40 AM, Nikolay Sivov wrote: >>> On 10/28/2012 04:59, max(a)mtew.isa-geek.net wrote: >>>> From: Max TenEyck Woodbury <max(a)mtew.isa-geek.net> >>>> >>>> I have been looking at the Microsoft COM and related documentations >>>> and noticed that they emphatically recommend using the Interlocked... >>>> functions when manipulating reference counts. I managed to set up a >>>> search that showed where many of the reference count updates occur and >>>> was somewhat surprised at how often this advice was not followed. >>> >>> It doesn't mean it always has to be followed. >> >> True in a limited sense, but there is a good reason behind their >> recommendation. Unless there is a good reason not to do so, this >> particular piece of advice should be followed. > > COM objects in wine follow this recommendation in general, even object > itself is not thread safe. > This doesn't mean however that you need this every time you have some > kind of refcount of any sort. It may or may not be necessary every time, but it should be demonstrated that it is not necessary rather than assumed that it is not. This is a 'race condition' after all, and they are known to be rare and difficult to isolate. I think it is good practice to assume there could be a race problem rather than otherwise. >> >>>> While I have not converted every reference count update to use the >>>> Interlocked... functions, this set of patches fixes a fair number of >>>> them. >>>> >>>> These are not associated with any particular bug report; they are >>>> simply a general precaution against operations that are known to be >>>> associated with race conditions. >>> >>> This precaution doesn't work in general. It's not enough to atomically >>> update refcount to make an implementation thread safe. Also not >>> everything is supposed to be thread safe in a first place. >>> >> First, explain what does not have to be thread safe. > > Anything really, COM objects in particular if you were talking about them. > I think you are talking about the apartment model here, which forces thread serialization. Despite that, the Microsoft documentation still recommends interlocked operations on reference counts... >> I believe that application may try to use multiple threads anywhere, >> so everything that can be made thread safe, should be. > No. What do you mean 'No'. That is an opinion, If you disagree, please explain why. >> Using interlocks on the reference count updates is a necessary step >> for thread safety. You are correct that it is not sufficient, but >> it is necessary. > Again, it depends. Yes, it might depend on many factors, most of which will make it safe to not interlock, but it is a lot of work checking that all those factors necessary to not use the interlock are in fact in place. Further, if enough of those factors get changed, you have a problem. This is what make code fragile. Fragile can be avoided with steps like this. >> >>> Changes like this: >>> >>>> - for (i=0;i<howmuch;i++) >>>> + for (i=0;i<howmuch;++i) >>>> TRACE("notify at %d to %p\n", >>>> notify[i].dwOffset,notify[i].hEventNotify); >>> >>> are not helpful at all. >>> >> The post increment and decrement operation are specified as saving >> the original value for use in the evaluation of the expression they >> are part of and modifying the underlying stored value. In expressions >> like this, that saved value is then discarded. The optimization phase >> of the compilation usually removes both the save and discard operations. > > Sure, but I don't think it's enough to justify such changes all over the > place, in existing code. > I agree that it is not enough to justify a separate set of patches, but as part of another set of changes, I think it is justified. After all, how else are examples of bad code going to be removed.

Oct. 29, 2012

Re: [PATCH] Potential reference count races

by Max TenEyck Woodbury

On 10/28/2012 12:07 PM, Alexandre Julliard wrote: > Nikolay Sivov <bunglehead(a)gmail.com> writes: > >> On 10/28/2012 17:44, Max TenEyck Woodbury wrote: >>> Specifying the unnecessary use of a temporary store is a bad habit to >>> have. You should tell the compiler exactly what needs to be done, not >>> throw in extraneous operations. >>> >>> So, the use of a post operator where a prefix operator is sufficient, >>> while almost certainly harmless, is still technically a mistake. > > You must be joking. > Joking? Only sort of. Just being my usual wryly pedantic self... :-)

Oct. 29, 2012

on UTF-8, other Unicode encodings and WinAPI

by Saulius Krasuckas

Proposal with probably several insights: http://www.utf8everywhere.org/ Lyrical, older version: http://www.joelonsoftware.com/articles/Unicode.html S.

Oct. 28, 2012

Re: [PATCH] Remove potential reference count races

by Nikolay Sivov

On 10/28/2012 18:00, Max TenEyck Woodbury wrote: > On 10/28/2012 03:13 AM, Nikolay Sivov wrote: >> On 10/28/2012 04:59, max(a)mtew.isa-geek.net wrote: >>> From: Max TenEyck Woodbury <max(a)mtew.isa-geek.net> >>> >>> --- >>> dlls/shlwapi/thread.c | 4 ++-- >>> 1 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/dlls/shlwapi/thread.c b/dlls/shlwapi/thread.c >>> index eb2c35d..43e0433 100644 >>> --- a/dlls/shlwapi/thread.c >>> +++ b/dlls/shlwapi/thread.c >>> @@ -157,7 +157,7 @@ static ULONG WINAPI threadref_AddRef(IUnknown *iface) >>> threadref * This = impl_from_IUnknown(iface); >>> TRACE("(%p)\n", This); >>> - return InterlockedIncrement(This->ref); >>> + return InterlockedIncrement(&This->ref); >>> } >>> static ULONG WINAPI threadref_Release(IUnknown *iface) >>> @@ -167,7 +167,7 @@ static ULONG WINAPI threadref_Release(IUnknown >>> *iface) >>> TRACE("(%p)\n", This); >>> - refcount = InterlockedDecrement(This->ref); >>> + refcount = InterlockedDecrement(&This->ref); >>> if (!refcount) >>> HeapFree(GetProcessHeap(), 0, This); >>> >> Did you try to build this? >> >> > Yes, it built. I don't submit stuff that causes the compiler to barf > unless I am completely totally zonked from lack of sleep and this was > not what happened here. I will admit that I have not looked at the > build and test logs closely, so there may be problems I missed. > These changes I wrong that's what I'm saying. P.S. please CC wine-devel list in your replies.

Oct. 28, 2012

Re: [PATCH] Potential reference count races

by Alexandre Julliard

Nikolay Sivov <bunglehead(a)gmail.com> writes: > On 10/28/2012 17:44, Max TenEyck Woodbury wrote: >> Specifying the unnecessary use of a temporary store is a bad habit to >> have. You should tell the compiler exactly what needs to be done, not >> throw in extraneous operations. >> >> So, the use of a post operator where a prefix operator is sufficient, >> while almost certainly harmless, is still technically a mistake. You must be joking. -- Alexandre Julliard julliard(a)winehq.org

Oct. 28, 2012