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?