June 2022 - Wine-Devel - list.winehq.org

Re: [PATCH v3 0/7] MR205: Add support for HRESULT and ID3DX10ThreadPump parameters in D3DX10GetImageInfo functions

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/tests/d3dx10.c: > } > > hr2 = 0xdeadbeef; > - SET_EXPECT(D3DX10ThreadPump_AddWorkItem); > + add_work_item_count = 0; > hr = D3DX10GetImageInfoFromMemory(test_image[0].data, test_image[0].size, &thread_pump, &image_info, &hr2); > - CHECK_CALLED(D3DX10ThreadPump_AddWorkItem); > + ok(add_work_item_count, "D3DX10ThreadPump_AddWorkItem not called\n"); Any reason not to check explicitly for == 1? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/205#note_1857

June 10, 2022

Re: [PATCH 0/3] MR225: server: Fix races related to socket errors

by Piotr Caban (＠piotr)

On Fri Jun 10 16:53:21 2022 +0000, Jinoh Kang wrote: > Do we want the following instead? > ```suggestion:-0+0 > *error = sock->errors[AFD_POLL_BIT_ACCEPT]; > ``` Yes, that was the intention, I'll send fixed version. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/225#note_1856

June 10, 2022

Re: [PATCH v2 4/4] advapi32: Implement RegRenameKey().

by Marvin

Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=116707 Your paranoid android. === w864 (64 bit report) === advapi32: registry.c:3471: Test failed: expected ERROR_SUCCESS, got 6 registry.c:3599: Test failed: expected WAIT_OBJECT_0, got 258

June 10, 2022

Re: [PATCH v2 1/2] ws2_32: Make wait in select() alertable.

by Marvin

Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=116718 Your paranoid android. === w1064v1507 (64 bit report) === ws2_32: sock.c:7966: Test failed: failed to accept, error 10035 sock.c:7978: Test failed: got 0xc0000023 sock.c:8013: Test failed: got 00000000 sock.c:8027: Test failed: AcceptEx on too small remote address size returned 0 + errno 10022 sock.c:8047: Test failed: AcceptEx returned 0 + errno 10022 sock.c:8067: Test failed: got error 10056 sock: Timeout

June 10, 2022

Re: [PATCH 0/3] MR225: server: Fix races related to socket errors

by Jinoh Kang (＠iamahuman)

Jinoh Kang (@iamahuman) commented about server/sock.c: > + if (getsockopt( get_unix_fd(sock->fd), SOL_SOCKET, SO_ERROR, (void *)error, &len) < 0) > + return -1; > + > + if (sock->state == SOCK_CONNECTING) > + { > + if (*error) > + sock->errors[AFD_POLL_BIT_CONNECT_ERR] = *error; > + else > + *error = sock->errors[AFD_POLL_BIT_CONNECT_ERR]; > + } > + else if (sock->state == SOCK_LISTENING) > + { > + if (*error) > + sock->errors[AFD_POLL_BIT_ACCEPT] = *error; > + else > + *error = sock->errors[AFD_POLL_BIT_CONNECT_ERR]; Do we want the following instead? ```suggestion:-0+0 *error = sock->errors[AFD_POLL_BIT_ACCEPT]; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/225#note_1853

June 10, 2022

Re: [PATCH v2 0/2] MR127: ws2_32: Make wait in select and recv alertable - approved

by Zebediah Figura (＠zfigura)

This merge request was approved by Zebediah Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/127

June 10, 2022

[PATCH v2 2/2] ws2_32: Make wait in WS2_recv_base() alertable.

by Paul Gofman

From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/socket.c | 2 +- dlls/ws2_32/tests/sock.c | 75 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 70 insertions(+), 7 deletions(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 9be7190062f..9cc75aacf20 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -959,7 +959,7 @@ static int WS2_recv_base( SOCKET s, WSABUF *buffers, DWORD buffer_count, DWORD * IOCTL_AFD_WINE_RECVMSG, &params, sizeof(params), NULL, 0 ); if (status == STATUS_PENDING && !overlapped) { - if (WaitForSingleObject( event, INFINITE ) == WAIT_FAILED) + if (wait_event_alertable( event ) == WAIT_FAILED) return -1; status = piosb->u.Status; } diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 80d458b650d..af4226e6258 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -6747,21 +6747,58 @@ static void test_WSASendTo(void) ok(ret_addr.sin_port, "expected nonzero port\n"); } +struct recv_thread_apc_param +{ + SOCKET sock; + unsigned int apc_count; +}; + +static void WINAPI recv_thread_apc_func(ULONG_PTR param) +{ + struct recv_thread_apc_param *p = (struct recv_thread_apc_param *)param; + int ret; + + ++p->apc_count; + + ret = send(p->sock, "test", 4, 0); + ok(ret == 4, "got %d.\n", ret); +} + +struct recv_thread_param +{ + SOCKET sock; + BOOL overlapped; +}; + static DWORD WINAPI recv_thread(LPVOID arg) { - SOCKET sock = *(SOCKET *)arg; + struct recv_thread_param *p = arg; + SOCKET sock = p->sock; char buffer[32]; WSABUF wsa; WSAOVERLAPPED ov; DWORD flags = 0; + DWORD len; + int ret; wsa.buf = buffer; wsa.len = sizeof(buffer); - ov.hEvent = WSACreateEvent(); - WSARecv(sock, &wsa, 1, NULL, &flags, &ov, NULL); + if (p->overlapped) + { + ov.hEvent = WSACreateEvent(); + WSARecv(sock, &wsa, 1, NULL, &flags, &ov, NULL); - WaitForSingleObject(ov.hEvent, 1000); - WSACloseEvent(ov.hEvent); + WaitForSingleObject(ov.hEvent, 1000); + WSACloseEvent(ov.hEvent); + } + else + { + SetLastError(0xdeadbeef); + ret = WSARecv(sock, &wsa, 1, &len, &flags, NULL, NULL); + ok(!ret, "got ret %d.\n", ret); + ok(WSAGetLastError() == 0, "got error %d.\n", WSAGetLastError()); + ok(len == 4, "got len %lu.\n", len); + } return 0; } @@ -6775,11 +6812,14 @@ static void WINAPI io_completion(DWORD error, DWORD transferred, WSAOVERLAPPED * static void test_WSARecv(void) { SOCKET src, dest, server = INVALID_SOCKET; + struct recv_thread_apc_param apc_param; + struct recv_thread_param recv_param; char buf[20]; WSABUF bufs[2]; WSAOVERLAPPED ov; DWORD bytesReturned, flags, id; struct sockaddr_in addr; + unsigned int apc_count; int iret, len; DWORD dwret; BOOL bret; @@ -6799,10 +6839,20 @@ static void test_WSARecv(void) ok(GetLastError() == ERROR_SUCCESS, "Expected 0, got %ld\n", GetLastError()); SetLastError(0xdeadbeef); bytesReturned = 0xdeadbeef; + + apc_count = 0; + dwret = QueueUserAPC(apc_func, GetCurrentThread(), (ULONG_PTR)&apc_count); + ok(dwret, "QueueUserAPC returned %lu\n", dwret); + iret = WSARecv(dest, bufs, 1, &bytesReturned, &flags, NULL, NULL); ok(!iret, "Expected 0, got %d\n", iret); ok(bytesReturned == 2, "Expected 2, got %ld\n", bytesReturned); ok(GetLastError() == ERROR_SUCCESS, "Expected 0, got %ld\n", GetLastError()); + + ok(!apc_count, "got apc_count %u.\n", apc_count); + SleepEx(0, TRUE); + ok(apc_count == 1, "got apc_count %u.\n", apc_count); + SetLastError(0xdeadbeef); bytesReturned = 0xdeadbeef; iret = WSARecv(dest, bufs, 1, &bytesReturned, &flags, NULL, NULL); @@ -6895,10 +6945,23 @@ static void test_WSARecv(void) if (dest == INVALID_SOCKET) goto end; send(src, "test message", sizeof("test message"), 0); - thread = CreateThread(NULL, 0, recv_thread, &dest, 0, &id); + recv_param.sock = dest; + recv_param.overlapped = TRUE; + thread = CreateThread(NULL, 0, recv_thread, &recv_param, 0, &id); WaitForSingleObject(thread, 3000); CloseHandle(thread); + recv_param.overlapped = FALSE; + thread = CreateThread(NULL, 0, recv_thread, &recv_param, 0, &id); + apc_param.apc_count = 0; + apc_param.sock = src; + dwret = QueueUserAPC(recv_thread_apc_func, thread, (ULONG_PTR)&apc_param); + ok(dwret, "QueueUserAPC returned %lu\n", dwret); + WaitForSingleObject(thread, 3000); + ok(apc_param.apc_count == 1, "got apc_count %u.\n", apc_param.apc_count); + + CloseHandle(thread); + memset(&ov, 0, sizeof(ov)); ov.hEvent = event; ResetEvent(event); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/127

June 10, 2022

[PATCH v2 1/2] ws2_32: Make wait in select() alertable.

by Paul Gofman

From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ws2_32/socket.c | 10 +++++++++- dlls/ws2_32/tests/sock.c | 16 ++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 964724b36e7..9be7190062f 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -551,6 +551,14 @@ static HANDLE get_sync_event(void) return data->sync_event; } +static DWORD wait_event_alertable( HANDLE event ) +{ + DWORD ret; + + while ((ret = WaitForSingleObjectEx( event, INFINITE, TRUE )) == WAIT_IO_COMPLETION) + ; + return ret; +} BOOL WINAPI DllMain( HINSTANCE instance, DWORD reason, void *reserved ) { @@ -2769,7 +2777,7 @@ int WINAPI select( int count, fd_set *read_ptr, fd_set *write_ptr, IOCTL_AFD_POLL, params, params_size, params, params_size ); if (status == STATUS_PENDING) { - if (WaitForSingleObject( sync_event, INFINITE ) == WAIT_FAILED) + if (wait_event_alertable( sync_event ) == WAIT_FAILED) { free( read_input ); free( params ); diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 22e414e02ec..80d458b650d 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -202,6 +202,11 @@ static void tcp_socketpair(SOCKET *src, SOCKET *dst) tcp_socketpair_flags(src, dst, WSA_FLAG_OVERLAPPED); } +static void WINAPI apc_func(ULONG_PTR apc_count) +{ + ++*(unsigned int *)apc_count; +} + /* Set the linger timeout to zero and close the socket. This will trigger an * RST on the connection on Windows as well as on Unix systems. */ static void close_with_rst(SOCKET s) @@ -3613,6 +3618,7 @@ static void test_select(void) select_thread_params thread_params; HANDLE thread_handle; DWORD ticks, id, old_protect; + unsigned int apc_count; unsigned int maxfd, i; char *page_pair; @@ -3716,14 +3722,24 @@ static void test_select(void) FD_ZERO(&readfds); FD_SET(fdRead, &readfds); + apc_count = 0; + ret = QueueUserAPC(apc_func, GetCurrentThread(), (ULONG_PTR)&apc_count); + ok(ret, "QueueUserAPC returned %d\n", ret); ret = select(fdRead+1, &readfds, NULL, NULL, &select_timeout); ok(!ret, "select returned %d\n", ret); + ok(apc_count == 1, "got apc_count %d.\n", apc_count); FD_ZERO(&writefds); FD_SET(fdWrite, &writefds); + apc_count = 0; + ret = QueueUserAPC(apc_func, GetCurrentThread(), (ULONG_PTR)&apc_count); + ok(ret, "QueueUserAPC returned %d\n", ret); ret = select(fdWrite+1, NULL, &writefds, NULL, &select_timeout); ok(ret == 1, "select returned %d\n", ret); ok(FD_ISSET(fdWrite, &writefds), "fdWrite socket is not in the set\n"); + ok(!apc_count, "APC was called\n"); + SleepEx(0, TRUE); + ok(apc_count == 1, "got apc_count %d.\n", apc_count); /* select the same socket twice */ writefds.fd_count = 2; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/127

June 10, 2022

[PATCH v2 0/2] MR127: ws2_32: Make wait in select alertable

by Paul Gofman (＠gofman)

Fixes V Rising hang on exit. During exit the main thread wants to terminate the other thread by queuing APC which APC is supposed to raise an exception and abort the thread. That is basically how Unity / Mono aborts threads. The thread being terminated is calling select() in a loop and the wait in select() is currently not alertable (while they should be, as the test suggests). But with the first patch the things may get more interesting. ws2_32.select() passes on stack io status block to NtDeviceIoControlFile(), then it is killed from APC while waiting for event to be signaled by ioctl async processing. Then, wineserver wants to complete the async. The async owner thread is already terminated and it delivers SIGUSR1 to process the system APC to the main thread. When the APC is processed from USR1 handler it segfaults trying to put the IO status to iosb pointing to already deallocated thread stack. This is segfault on signal stack and as such is unrecoverable. The process is left hanging. Thus the second patch. In the ideal case we'd probably want to avoid crashing or handle segfault in system APC processing somehow, but I don't yet see a feasible way to do it. In theory the same may happen without ws2_32 if the app does the same, but I don't know that anything hits that case while ws2_32 queues those on stack iosbs, so avoiding that may probably avoid the practical issues. -- v2: ws2_32: Make wait in WS2_recv_base() alertable. ws2_32: Make wait in select() alertable. https://gitlab.winehq.org/wine/wine/-/merge_requests/127

June 10, 2022

Re: Questions about the GitLab patch review process

by Rémi Bernon

On 6/10/22 17:21, Alex Henrie wrote: > Dear developers, > > I have a couple of questions about how patch reviews on GitLab are > intended to work: > > * Can I request a review from anyone? > > * Does requesting a review improve the patch's chances of being accepted? > > Thanks for your help. > > -Alex > Imho yes to both, especially to the second question. For the first question, it's probably best to request review from the maintainer of a module. Sometimes there isn't any and looking for someone knowledgeable is best. If there is a maintainer, they will probably be assigned by Alexandre anyway at some point (but it saves some effort if you do). -- Rémi Bernon <rbernon(a)codeweavers.com>

June 10, 2022