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

Re: [PATCH] Potential reference count races

by Nikolay Sivov

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. > >>> 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 believe that > application may try to use multiple threads anywhere, so everything > that can be made thread safe, should be. No. > 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. > >> 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. > > Murphy's law suggests that this might cause problems at some point when > least desired. Please don't do this. > > 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. > > At the minimum, these changes provide examples of the correct way these > statements should be coded. So, I have to disagree, although very > mildly, that changes like that are not helpful. This kind of change > does not deserve a separate patch, but should be perfectly acceptable as > an adjunct to other patches.

Oct. 28, 2012

Re: VBSCRIPT.DLL errors with CorelDRAW 12 installer (Wine 1.5.15)

by André Hentschel

Am 28.10.2012 09:06, schrieb Gerold Jens Wucherpfennig: > (I just bought Windows 8, > so in return I want to spend some time > and money to wine...) > One easy thing you could do is to run the testsuite on win8 (see the bottom of http://test.winehq.org/) > fixme:vbscript:parse_script parser failed on parsing L"1\r\nConst > msiInstallStateAdvertise = 1\r\nConst msiInstallStateAbsent = 2\r\nConst > msiInstallStateLocal = 3\r\nConst msiInstallStateSource = 4\r\nConst > msiInstallStateDefault = 5\r\nDim nRemoveVBAEN:nRemoveVBAEN = 0\r\nDim > nRemoveVBACZ:nRemoveVBACZ = 0\r\nDim nRemoveVBANL:nRemoveVBAPL = > 0\r\n\r\nOn "... As vbscript knows about "Const" i'd say the parser starts with parsing a single "1" and fails. Not sure if that's the biggest problem for the installer, as i see much ERRs in the output. -- Best Regards, André Hentschel

Oct. 28, 2012

Re: VBSCRIPT.DLL errors with CorelDRAW 12 installer (Wine 1.1.15)

by Nikolay Sivov

On 10/28/2012 10:59, Gerold Jens Wucherpfennig wrote: > Hi, can someone help me with CorelDRAW 12: > > http://appdb.winehq.org/objectManager.php?sClass=version&iId=2390&iTestingI… > > Wine 1.5.15 vanilla without external DLLs > > > Output from setup.exe in output.txt > (only uniq lines saved; error code 67 on exit) Please open a bug report for this at bugs.winehq.org.

Oct. 28, 2012

VBSCRIPT.DLL errors with CorelDRAW 12 installer (Wine 1.5.15)

by Gerold Jens Wucherpfennig

(I just bought Windows 8, so in return I want to spend some time and money to wine...) Hi, can someone help me with CorelDRAW 12: http://appdb.winehq.org/objectManager.php?sClass=version&iId=2390&iTestingI… Wine 1.5.15 vanilla without external DLLs Output from setup.exe in output.txt (only uniq lines saved; error code 67 on exit) vbscript errors (lines copied from output.txt): fixme:vbscript:parse_script parser failed on parsing L"1\r\nConst msiInstallStateAdvertise = 1\r\nConst msiInstallStateAbsent = 2\r\nConst msiInstallStateLocal = 3\r\nConst msiInstallStateSource = 4\r\nConst msiInstallStateDefault = 5\r\nDim nRemoveVBAEN:nRemoveVBAEN = 0\r\nDim nRemoveVBACZ:nRemoveVBACZ = 0\r\nDim nRemoveVBANL:nRemoveVBAPL = 0\r\n\r\nOn "... fixme:vbscript:parse_script parser failed on parsing L"1\r\nConst msiInstallStateAdvertise = 1\r\nConst msiInstallStateAbsent = 2\r\nConst msiInstallStateLocal = 3\r\nConst msiInstallStateSource = 4\r\nConst msiInstallStateDefault = 5\r\nDim nRemoveVBAEN:nRemoveVBAEN = 0\r\nDim nRemoveVBACZ:nRemoveVBACZ = 0\r\nDim nRemoveVBANL:nRemoveVBAPL = 0\r\n\r\nOn "... fixme:vbscript:parse_script parser failed on parsing L"1\r\nConst msiInstallStateAdvertise = 1\r\nConst msiInstallStateAbsent = 2\r\nConst msiInstallStateLocal = 3\r\nConst msiInstallStateSource = 4\r\nConst msiInstallStateDefault = 5\r\n\r\nOn Error Resume Next\r\n\r\nIf (IsFeatureSelected(\"FontNav\") = True) Then\r\n If ((Session.Property(\"PLLANG\""... fixme:vbscript:parse_script parser failed on parsing L"1\r\nConst msiInstallStateAdvertise = 1\r\nConst msiInstallStateAbsent = 2\r\nConst msiInstallStateLocal = 3\r\nConst msiInstallStateSource = 4\r\nConst msiInstallStateDefault = 5\r\n\r\nOn Error Resume Next\r\n\r\nIf (IsFeatureSelected(\"FontNav\") = True) Then\r\n If ((Session.Property(\"PLLANG\""... fixme:vbscript:parse_script parser failed on parsing L"1\r\nConst msiInstallStateAdvertise = 1\r\nConst msiInstallStateAbsent = 2\r\nConst msiInstallStateLocal = 3\r\nConst msiInstallStateSource = 4\r\nConst msiInstallStateDefault = 5\r\n\r\nOn Error Resume Next\r\n\t\r\nIf (IsFeatureExists(\"LocalInstallsOnly\")=\"1\") Then\r\n\tIf ((Session.Property(\""... Best Regards, Gerold

Oct. 28, 2012

VBSCRIPT.DLL errors with CorelDRAW 12 installer (Wine 1.1.15)

by Gerold Jens Wucherpfennig

Hi, can someone help me with CorelDRAW 12: http://appdb.winehq.org/objectManager.php?sClass=version&iId=2390&iTestingI… Wine 1.5.15 vanilla without external DLLs Output from setup.exe in output.txt (only uniq lines saved; error code 67 on exit) vbscript errors (lines copied from output.txt): fixme:vbscript:parse_script parser failed on parsing L"1\r\nConst msiInstallStateAdvertise = 1\r\nConst msiInstallStateAbsent = 2\r\nConst msiInstallStateLocal = 3\r\nConst msiInstallStateSource = 4\r\nConst msiInstallStateDefault = 5\r\nDim nRemoveVBAEN:nRemoveVBAEN = 0\r\nDim nRemoveVBACZ:nRemoveVBACZ = 0\r\nDim nRemoveVBANL:nRemoveVBAPL = 0\r\n\r\nOn "... fixme:vbscript:parse_script parser failed on parsing L"1\r\nConst msiInstallStateAdvertise = 1\r\nConst msiInstallStateAbsent = 2\r\nConst msiInstallStateLocal = 3\r\nConst msiInstallStateSource = 4\r\nConst msiInstallStateDefault = 5\r\nDim nRemoveVBAEN:nRemoveVBAEN = 0\r\nDim nRemoveVBACZ:nRemoveVBACZ = 0\r\nDim nRemoveVBANL:nRemoveVBAPL = 0\r\n\r\nOn "... fixme:vbscript:parse_script parser failed on parsing L"1\r\nConst msiInstallStateAdvertise = 1\r\nConst msiInstallStateAbsent = 2\r\nConst msiInstallStateLocal = 3\r\nConst msiInstallStateSource = 4\r\nConst msiInstallStateDefault = 5\r\n\r\nOn Error Resume Next\r\n\r\nIf (IsFeatureSelected(\"FontNav\") = True) Then\r\n If ((Session.Property(\"PLLANG\""... fixme:vbscript:parse_script parser failed on parsing L"1\r\nConst msiInstallStateAdvertise = 1\r\nConst msiInstallStateAbsent = 2\r\nConst msiInstallStateLocal = 3\r\nConst msiInstallStateSource = 4\r\nConst msiInstallStateDefault = 5\r\n\r\nOn Error Resume Next\r\n\r\nIf (IsFeatureSelected(\"FontNav\") = True) Then\r\n If ((Session.Property(\"PLLANG\""... fixme:vbscript:parse_script parser failed on parsing L"1\r\nConst msiInstallStateAdvertise = 1\r\nConst msiInstallStateAbsent = 2\r\nConst msiInstallStateLocal = 3\r\nConst msiInstallStateSource = 4\r\nConst msiInstallStateDefault = 5\r\n\r\nOn Error Resume Next\r\n\t\r\nIf (IsFeatureExists(\"LocalInstallsOnly\")=\"1\") Then\r\n\tIf ((Session.Property(\""... Best Regards, Gerold

Oct. 28, 2012

Re: [PATCH] Remove potential reference count races

by Nikolay Sivov

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?

Oct. 28, 2012

Re: [PATCH] Remove potential reference count races

by Nikolay Sivov

On 10/28/2012 04:59, max(a)mtew.isa-geek.net wrote: > From: Max TenEyck Woodbury <max(a)mtew.isa-geek.net> > > --- > dlls/jscript/engine.h | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > Take a look at http://source.winehq.org/git/wine.git/commit/79a30a4e0ae3a7ef640fa38e3a9f77…

Oct. 28, 2012

Re: [PATCH] Potential reference count races

by Nikolay Sivov

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 recomend 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. > > 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 precausion 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. 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 patches are independent of each other. They are NOT a series. >

Oct. 28, 2012

Re: [PATCH] Remove potential reference count races.

by Nikolay Sivov

On 10/28/2012 04:59, max(a)mtew.isa-geek.net wrote: > From: Max TenEyck Woodbury <max(a)mtew.isa-geek.net> > > --- > dlls/gdi32/freetype.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/dlls/gdi32/freetype.c b/dlls/gdi32/freetype.c > index 6af977c..3379c6e 100644 > --- a/dlls/gdi32/freetype.c > +++ b/dlls/gdi32/freetype.c > @@ -3820,7 +3820,7 @@ static struct font_mapping *map_font_file( const char *name ) > { > if (mapping->dev == st.st_dev && mapping->ino == st.st_ino) > { > - mapping->refcount++; > + InterlockedIncrement(&mapping->refcount); > close( fd ); > return mapping; > } > @@ -3850,7 +3850,7 @@ error: > > static void unmap_font_file( struct font_mapping *mapping ) > { > - if (!--mapping->refcount) > + if (!InterlockedDecrement(&mapping->refcount)) > { > list_remove( &mapping->entry ); > munmap( mapping->data, mapping->size ); How does it help exactly and what is the problem with this code?

Oct. 28, 2012

Re: [PATCH] wininet: TLS fallback mechanism (take2)

by Hiroshi Miura

Hi, My updated patch which is Message-ID: <5089BDA7.3040806(a)linux.com> is not properly treated in http://source.winehq.org/patches http://source.winehq.org/patches/data/91337 New one is removed, and the one that make compile error in older openssl is tagged as new. Hiroshi Miura wrote: > OpenSSL 1.0.x now support TLSv1.1 and TLSv1.2. > Sometimes TLSv1.1/1.2 negotiation fails bacause of combination > of server/client ssl library versions. > It fixes the error connecting Evernote server with evernote client. > http://bugs.winehq.org/show_bug.cgi?id=30598 > > It has also mechanism enable/disable TLSv1.1/1.2 by registry entry under > SYSTEM/CurrentControlSet/Control/SecurityProvider/SCHANNEL/Protocols/ > where is as same place as Windows7. > TLSv1.1/1.2 is enabled when the registry entry does not exist. > > Signed-off-by: Hiroshi Miura <miurahr(a)linux.com> > --- > dlls/wininet/netconnection.c | 169 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 130 insertions(+), 39 deletions(-) > > diff --git a/dlls/wininet/netconnection.c b/dlls/wininet/netconnection.c > index c944ff3..9b5f7e7 100644 > --- a/dlls/wininet/netconnection.c > +++ b/dlls/wininet/netconnection.c <snip> > +#ifdef SSL_OP_NO_TLSv1_2 > +static long get_tls_option(void) { > + long tls_option; <snip> Hiroshi

Oct. 27, 2012