August 2017 - Wine-devel - list.winehq.org

Re: [4/6] wsdapi: Implement "Hello" message creation support. [try 2]
by Vincent Povirk 02 Aug '17

02 Aug '17

You're still using rand/srand in this one.

2 1

Re: [2/6] wsdapi: Add code to create SOAP envelope from header and body elements. [try 2]
by Vincent Povirk 02 Aug '17

02 Aug '17

+static void remove_attribute(WSDXML_ELEMENT *parent, WSDXML_ATTRIBUTE *attribute) +{ + WSDXML_ATTRIBUTE *curAttrib; + + /* Find the last attribute and add this as the next one */ + curAttrib = parent->FirstAttribute; + + if (curAttrib == attribute) + { + parent->FirstAttribute = NULL; + } I think this should be parent->FirstAttribute = curAttrib->Next. + } + else if (curNode->Type == TextType) + { + textNode = WSDAllocateLinkedMemory(newElement, sizeof(WSDXML_TEXT)); + if (textNode == NULL) goto cleanup; + + textNode->Node.Next = NULL; + textNode->Node.Parent = newElement; + textNode->Node.Type = TextType; + textNode->Text = duplicate_string(textNode, ((WSDXML_TEXT *)curNode)->Text); + if (textNode->Text == NULL) goto cleanup; + } You still don't seem to be adding the new text node to newElement.

2 1

Re: [PATCH v2] ole32: Store proxy/stub CLSIDs per process, not per apartment.
by Huw Davies 02 Aug '17

02 Aug '17

On Tue, Aug 01, 2017 at 04:54:45PM -0500, Zebediah Figura wrote: > v2: use proper synchronization; add an MTA test (thanks Sebastian) > Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> > --- > dlls/ole32/compobj.c | 48 ++++++++++++++++++++++++++++++------------ > dlls/ole32/compobj_private.h | 1 - > dlls/ole32/tests/compobj.c | 50 ++++++++++++++++++++++++++++++++++++++++++-- > 3 files changed, 83 insertions(+), 16 deletions(-) > > diff --git a/dlls/ole32/compobj.c b/dlls/ole32/compobj.c > index 60244485249..ee904adbc38 100644 > --- a/dlls/ole32/compobj.c > +++ b/dlls/ole32/compobj.c > @@ -170,8 +170,20 @@ struct registered_psclsid > struct list entry; > IID iid; > CLSID clsid; > + OXID apartment_id; > }; > > +static struct list RegisteredPSCLSIDList = LIST_INIT(RegisteredPSCLSIDList); Please don't use CamelCase. > + > +static CRITICAL_SECTION csRegisteredPSCLSIDList; > +static CRITICAL_SECTION_DEBUG psclsid_cs_debug = > +{ > + 0, 0, &csRegisteredPSCLSIDList, > + { &psclsid_cs_debug.ProcessLocksList, &psclsid_cs_debug.ProcessLocksList }, > + 0, 0, { (DWORD_PTR)(__FILE__ ": csRegisteredPSCLSIDList") } > +}; > +static CRITICAL_SECTION csRegisteredPSCLSIDList = { &psclsid_cs_debug, -1, 0, 0, 0, 0 }; > + > /* > * This is a marshallable object exposing registered local servers. > * IServiceProvider is used only because it happens meet requirements > @@ -622,7 +634,6 @@ static APARTMENT *apartment_construct(DWORD model) > > list_init(&apt->proxies); > list_init(&apt->stubmgrs); > - list_init(&apt->psclsids); > list_init(&apt->loaded_dlls); > apt->ipidc = 0; > apt->refs = 1; > @@ -1173,15 +1184,22 @@ DWORD apartment_release(struct apartment *apt) > stub_manager_int_release(stubmgr); > } > > - LIST_FOR_EACH_SAFE(cursor, cursor2, &apt->psclsids) > + EnterCriticalSection(&csRegisteredPSCLSIDList); > + > + LIST_FOR_EACH_SAFE(cursor, cursor2, &RegisteredPSCLSIDList) > { > struct registered_psclsid *registered_psclsid = > LIST_ENTRY(cursor, struct registered_psclsid, entry); > > - list_remove(&registered_psclsid->entry); > - HeapFree(GetProcessHeap(), 0, registered_psclsid); > + if (registered_psclsid->apartment_id == apt->oxid) > + { > + list_remove(&registered_psclsid->entry); > + HeapFree(GetProcessHeap(), 0, registered_psclsid); > + } > } > > + LeaveCriticalSection(&csRegisteredPSCLSIDList); > + > /* if this assert fires, then another thread took a reference to a > * stub manager without taking a reference to the containing > * apartment, which it must do. */ It seems it's more complicated than this. Consider two already existing STAs, if the PSClsid is registered in STA1 which then leaves the apt, a call in STA2 to CoGetPSCLSID() still succeeds. You'll want to expand the tests to cover things like this to really figure out what's going on. Also note that the MTA -> MTA test isn't very convincing, since there is only one MTA and hence both threads are in the same apt. Huw.

2 1

Re: [PATCHv2] msvcrt: Export -f suffixed float functions functions on arm64
by Piotr Caban 02 Aug '17

02 Aug '17

Hi Martin, On 07/31/17 14:47, Martin Storsjo wrote: > --- a/dlls/api-ms-win-crt-math-l1-1-0/api-ms-win-crt-math-l1-1-0.spec > +++ b/dlls/api-ms-win-crt-math-l1-1-0/api-ms-win-crt-math-l1-1-0.spec > @@ -77,7 +77,7 @@ > @ cdecl _fdtest(ptr) ucrtbase._fdtest > @ stub _fdunscale > @ cdecl _finite(double) ucrtbase._finite > -@ cdecl -arch=arm,x86_64 _finitef(float) ucrtbase._finitef > +@ cdecl -arch=arm,x86_64,arm64 _finitef(float) ucrtbase._finitef > @ cdecl _fpclass(double) ucrtbase._fpclass > @ stub _fpclassf > @ cdecl -arch=i386 -ret64 _ftol() ucrtbase._ftol > @@ -85,7 +85,7 @@ > @ cdecl _hypot(double double) ucrtbase._hypot > @ cdecl _hypotf(float float) ucrtbase._hypotf > @ cdecl _isnan(double) ucrtbase._isnan > -@ cdecl -arch=arm,x86_64 _isnanf(float) ucrtbase._isnanf > +@ cdecl -arch=arm,x86_64,arm64 _isnanf(float) ucrtbase._isnanf Maybe I'm checking it incorrectly but I don't see isnanf in the lib file. Is it really exported on arm64? I didn't check all of the functions but _scalbf also seems to be not exported. Thanks, Piotr

2 1

Re: [PATCH 2/3] msvcrt: Set errno through the _matherr function.
by Piotr Caban 02 Aug '17

02 Aug '17

On 08/01/17 08:13, Alex Henrie wrote: > @@ -252,7 +320,7 @@ float CDECL MSVCRT_powf( float x, float y ) > { > /* FIXME: If x < 0 and y is not integral, set EDOM */ > float z = powf(x,y); > - if (!finitef(z)) *MSVCRT__errno() = MSVCRT_EDOM; > + if (!finitef(z)) math_error(_DOMAIN, "powf", x, 0, ret); ret is not defined here. Could you please also add exception.name tests (in this case it should be set to pow instead of powf). Thanks, Piotr

2 2

Re: [5/6] wsdapi: Add support for sending messages via UDP multicast.
by Vincent Povirk 01 Aug '17

01 Aug '17

+ delay = rand() % (UDP_MAX_DELAY - UDP_MIN_DELAY + 1) + UDP_MIN_DELAY; Using rand() for this doesn't seem very random, and it could interfere with an application's use of rand(). + /* Get size of buffer for adapters */ + adapterAddresses = (IP_ADAPTER_ADDRESSES *) HeapAlloc(heap, 0, bufferSize); Was this comment meant to be earlier? Sending messages to every adapter in sequence, with a delay for each, doesn't seem right. +void terminate_networking(IWSDiscoveryPublisherImpl *impl) +{ + impl->publisherStarted = FALSE; + + WSACleanup(); +} It seems like this will call WSACleanup even when freeing an object that was never used.

2 1

Re: [PATCH] ole32: Store proxy/stub CLSIDs per process, not per apartment.
by Sebastian Lackner 01 Aug '17

01 Aug '17

On 01.08.2017 23:27, Zebediah Figura wrote: > @@ -1173,13 +1175,16 @@ DWORD apartment_release(struct apartment *apt) > stub_manager_int_release(stubmgr); > } > > - LIST_FOR_EACH_SAFE(cursor, cursor2, &apt->psclsids) > + LIST_FOR_EACH_SAFE(cursor, cursor2, &RegisteredPSCLSIDList) Accessing a global list from multiple threads requires synchronization (for example, using a CS to avoid conflicting access). Using apt->cs is not sufficient because it might be a different one for each thread. The same also applies to all other places where the list is used. > @@ -1177,6 +1196,15 @@ static void test_CoRegisterPSClsid(void) > hr = CoRegisterPSClsid(&IID_IWineTest, &CLSID_WineTestPSFactoryBuffer); > ok_ole_success(hr, "CoRegisterPSClsid"); > > + hr = CoGetPSClsid(&IID_IWineTest, &clsid); > + ok_ole_success(hr, "CoGetPSClsid"); > + ok(IsEqualGUID(&clsid, &CLSID_WineTestPSFactoryBuffer), "expected %s, got %s\n", > + wine_dbgstr_guid(&CLSID_WineTestPSFactoryBuffer), wine_dbgstr_guid(&clsid)); > + > + thread = CreateThread(NULL, 0, register_ps_clsid_thread, NULL, 0, &tid); > + ok(thread != NULL, "CreateThread failed with error %d\n", GetLastError()); > + ok(!WaitForSingleObject(thread, 10000), "wait timed out\n"); The thread handle is leaked here. Also, it would be more convincing to run the tests for both STA and MTA.

1 0

Re: [2/6] wsdapi: Add code to create SOAP envelope from header and body elements.
by Vincent Povirk 01 Aug '17

01 Aug '17

+ wideStringLength = lstrlenW(wideString); + sizeNeeded = WideCharToMultiByte(CP_UTF8, 0, wideString, wideStringLength, NULL, 0, NULL, NULL); + + if (sizeNeeded < 0) + { + return NULL; + } + + newString = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeNeeded + 1); + WideCharToMultiByte(CP_UTF8, 0, wideString, wideStringLength, newString, sizeNeeded, NULL, NULL); This would be much simpler if you passed in -1 as the wide string length. Using HEAP_ZERO_MEMORY to null-terminate the string is a bit weird. + else if (curNode->Type == TextType) + { + textNode = WSDAllocateLinkedMemory(parent, sizeof(WSDXML_TEXT)); + if (textNode == NULL) goto cleanup; + + textNode->Node.Next = NULL; + textNode->Node.Parent = parent; + textNode->Node.Type = TextType; + textNode->Text = duplicate_string(textNode, ((WSDXML_TEXT *)curNode)->Text); + if (textNode->Text == NULL) goto cleanup; + } I'm not sure, but I think you need to add textNode as a child of newElement? + /* SequenceID attribute */ + if (header->AppSequence->SequenceId != NULL) + { + if (!add_string_attribute(xmlContext, appSequenceElement, discoveryNsUri, sequenceIdString, header->AppSequence->SequenceId)) goto cleanup; + } Do we need the namespace for SequenceID?

2 3

Re: [PATCH 1/2] winebrowser: Try other browsers on non-zero exit code.
by Vincent Povirk 01 Aug '17

01 Aug '17

Using system is usually a bad idea for security reasons. I also don't think waiting for the browser command to return makes sense. It may keep winebrowser running longer than expected, and we don't know for sure what the return value means.

2 1

Re: [PATCH 5/6 v3] ws2_32: Added support for overlapped GetAddrInfoExW.
by Sebastian Lackner 01 Aug '17

01 Aug '17

On 01.08.2017 19:08, Jacek Caban wrote: > + > + overlapped->Internal = ret; > + if (overlapped->hEvent) SetEvent(overlapped->hEvent); Not sure how it is implemented on Windows, but caching overlapped->hEvent would ensure that it also works when the overlapped struct is immediately reused after ->Internal changed.

1 0