[PATCH 0/1] MR1121: winhttp: Fix missing reference upcount
The reference count of host is supposed to go up by one in netconn_create. The missing ref increment causes error handling paths where netconn_close or release_host is called to prematurely decrement the ref count of host to zero. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1121
From: Yuan Yao <yaoyuan.0553(a)bytedance.com> The reference count of host is supposed to go up by one in netconn_create. The missing increment causes error handling paths where netconn_close or release_host is called to prematurely decrement the ref count of host to zero. --- dlls/winhttp/request.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 64f70b61a96..f6f6be38b7f 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -1635,6 +1635,9 @@ static DWORD open_connection( struct request *request ) len = lstrlenW( addressW ) + 1; send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_CONNECTING_TO_SERVER, addressW, len ); + EnterCriticalSection( &connection_pool_cs ); + ++host->ref; + LeaveCriticalSection( &connection_pool_cs ); if ((ret = netconn_create( host, &connect->sockaddr, request->connect_timeout, &netconn ))) { free( addressW ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1121
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125194 Your paranoid android. === debian11 (32 bit report) === d3d9: device.c:4255: Test failed: Expected message 0x7e for window 0, but didn't receive it, i=1. === debian11 (build log) === Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24726. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24726. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24726.
Why do you need to keep the host in the connection pool when an error occurs? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1121#note_11526
On Sun Oct 23 14:56:18 2022 +0000, Hans Leidekker wrote:
Why do you need to keep the host in the connection pool when an error occurs? After re-evaluating the logic and the intention behind the code here, I think you are correct, there is no need to add an extra reference count here.
The original problem occurred during QQ installation, a popular Chinese chatting App. During installation, a subprocess named QQSetupEx.exe accesses external resources using WinHttp APIs. The APIs are used asynchronously, i.e., it is set up by `WinHttpOpen` with `WINHTTP_FLAG_ASYNC` flag. Problem arises when the QQ program calls `WinHttpOpenRequest` and `WinHttpReadData` back to back. The scenario involves two threads, the main thread and thread 2 for the asynchronous call. Thread 2 starts to execute `WinHttpOpenRequest` and is blocked on [`netconn_secure_connect`](https://gitlab.winehq.org/wine/wine/-/blob/d4e68b1a868a1eed24446d98033388d5e...) due to another bug with parameters not reset before reuse. Later, the main thread invokes `WinHttpReadData`. [According to MSDN](https://learn.microsoft.com/en-us/windows/win32/api/winhttp/nf-winhttp-winht...), even when WinHttp is used in asynchronous mode, the function "can operate either synchronously or asynchronously". In this case, our wine implementation chooses the synchronous mode by having [`skip_async_queue`](https://gitlab.winehq.org/wine/wine/-/blob/d4e68b1a868a1eed24446d98033388d5e...) returning `true`. Consequently, when [`cache_connection`](https://gitlab.winehq.org/wine/wine/-/blob/d4e68b1a868a1eed24446d98033388d5e...) is invoked, the connection created in thread 2 and stored in `request` at [dlls/winhttp/request.c:1650](https://gitlab.winehq.org/wine/wine/-/blob/d4e68b1a868a1eed24446d98033388d5e...) is cached into `host->connections`. Now, thread 2 wakes up, and fails at `netconn_secure_connect`, going into the error recovery logic. Since `host->connections` is now non-empty, the assertion fails and crashes the program as shown below: ``` Assertion failed: list_empty( &host->connections ), file ../wine/dlls/winhttp/request.c, line 1435 ``` My complete fix is in another patch yet to be submitted. I now see that the reference count change is redundant and will actually cause memory leaks. I would like to take this chance and discuss my proposed change with you. Would you like it to be posted here or in a new merge request? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1121#note_11834
On Sun Oct 23 14:56:18 2022 +0000, Yuan Yao wrote:
After re-evaluating the logic and the intention behind the code here, I think you are correct, there is no need to add an extra reference count here. The original problem occurred during QQ installation, a popular Chinese chatting App. During installation, a subprocess named QQSetupEx.exe accesses external resources using WinHttp APIs. The APIs are used asynchronously, i.e., it is set up by `WinHttpOpen` with `WINHTTP_FLAG_ASYNC` flag. Problem arises when the QQ program calls `WinHttpOpenRequest` and `WinHttpReadData` back to back. The scenario involves two threads, the main thread and thread 2 for the asynchronous call. Thread 2 starts to execute `WinHttpOpenRequest` and is blocked on [`netconn_secure_connect`](https://gitlab.winehq.org/wine/wine/-/blob/d4e68b1a868a1eed24446d98033388d5e...) due to another bug with parameters not reset before reuse. Later, the main thread invokes `WinHttpReadData`. [According to MSDN](https://learn.microsoft.com/en-us/windows/win32/api/winhttp/nf-winhttp-winht...), even when WinHttp is used in asynchronous mode, the function "can operate either synchronously or asynchronously". In this case, our wine implementation chooses the synchronous mode by having [`skip_async_queue`](https://gitlab.winehq.org/wine/wine/-/blob/d4e68b1a868a1eed24446d98033388d5e...) returning `true`. Consequently, when [`cache_connection`](https://gitlab.winehq.org/wine/wine/-/blob/d4e68b1a868a1eed24446d98033388d5e...) is invoked, the connection created in thread 2 and stored in `request` at [dlls/winhttp/request.c:1650](https://gitlab.winehq.org/wine/wine/-/blob/d4e68b1a868a1eed24446d98033388d5e...) is cached into `host->connections`. Now, thread 2 wakes up, and fails at `netconn_secure_connect`, going into the error recovery logic. Since `host->connections` is now non-empty, the assertion fails and crashes the program as shown below: ``` Assertion failed: list_empty( &host->connections ), file ../wine/dlls/winhttp/request.c, line 1435 ``` My complete fix is in another patch yet to be submitted. I now see that the reference count change is redundant and will actually cause memory leaks. I would like to take this chance and discuss my proposed change with you. Would you like it to be posted here or in a new merge request? It's fine to post it here. You can change the subject to match the new proposal.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1121#note_11848
This merge request was closed by Hans Leidekker. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1121
participants (4)
-
Hans Leidekker (＠hans) -
Marvin -
Yuan Yao
-
Yuan Yao (＠yaoyuan.0553)