[PATCH 0/8] MR1422: winhttp: Do not reference request from websocket.
The initial motivation is to make WinHttpCloseHandle(request) with request previously upgraded to websocket deliver WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING right away and not after websocket is closed much later (this late notification causes crash in Microsoft Flight Simulator). That happens so as the notification is sent when the object is destroyed, not when the handle is closed. This logic seems correct to me: when testing some more things involving closing handles and observing which threads get the notifications I saw Windows delivering WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING in the other threads, which suggests that the notification can be queued and delayed on Windows too in case there are pending operations involving the object. So the right thing to do seems to be to have just the separate websocket and do not reference request from it. It probably deserves a separate explanation why I am adding a refcounting to netconn and copying it to webscoket instead of just nullifying the netconn in request. Doing that way (just nullifying the netconn in request) immediately caused regression in Elden Ring which started failing in a convoluted way: it doesn't wait for WebSocketReceiveResponse() to complete and calls WebSocketCompleteUpgrade(). That currently succeeds in Wine due to some lack of validation. Then, after upgrade, async WebSocketReceiveResponse proceeds, but with NULL netconn it gets an error from prior WebSocketReceiveResponse in callback and fails the thing. The core problem here is that unless recursive request is involved WebSocketReceiveResponse is always sync on Windows and, as my testing goes, does not involve network communication at all. I. e., looks like the actual reply receiving should be done in WebSocketSendRequest, WebSocketReceiveResponse only sends notifications and possibly queues recursive reque sts. I have a WIP patches for that and_with the help of those and existing winhttp test I realized that removing connection from request is wrong even without convoluted story. We have an existing test in winhttp.c which performs second request / upgrade on a request already upgraded socket. The test as is succeeds even with NULL connection in request by that is by chance only (as we don't try to receive response from our in process server yet which is failing in this case). With connection removed request opens a new one which doesn't get processed by our server, while in the same test on Windows I see that request is actually received by server, suggesting that it uses all the same existing connection on Windows, and so we should keep the same connection in request. As a separate note, I suspect that we now can erroneously cache connection for request upgraded to websocket. But that happens not on request close but under different conditions and looks the same before and after present patches, so this should be probably checked and fixed separately. Patch "winhttp: Move read buffer to websocket." might look weird as introducing the buffer for the only purpose of preserve initial data. While I don't immediately see how that can be done simplier, I have another small excuse for that. Currently the logic of closing websocket handles does the correct thing WRT notifications and aborting the operation. Yet it is still racy a bit due to we use app's buffer in socket read calls. If we happen to receive some data already after we sent notifications but before the read operation is aborted it may write to already freed or otherwise reused app's buffer. It seems to me that the only clean way to deal with that later is to use our own read buffer and copy the data only when we are going to send non-abort completion notification. So this read buffer might have more use in the future. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1422
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/net.c | 5 ++++- dlls/winhttp/request.c | 16 ++++++++-------- dlls/winhttp/winhttp_private.h | 3 ++- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/dlls/winhttp/net.c b/dlls/winhttp/net.c index ab94b5bfd1d..0652dfcb30a 100644 --- a/dlls/winhttp/net.c +++ b/dlls/winhttp/net.c @@ -214,6 +214,7 @@ DWORD netconn_create( struct hostdata *host, const struct sockaddr_storage *sock winsock_init(); if (!(conn = calloc( 1, sizeof(*conn) ))) return ERROR_OUTOFMEMORY; + conn->refs = 1; conn->host = host; conn->sockaddr = *sockaddr; if ((conn->socket = WSASocketW( sockaddr->ss_family, SOCK_STREAM, 0, NULL, 0, WSA_FLAG_OVERLAPPED )) == -1) @@ -277,8 +278,10 @@ DWORD netconn_create( struct hostdata *host, const struct sockaddr_storage *sock return ERROR_SUCCESS; } -void netconn_close( struct netconn *conn ) +void netconn_release( struct netconn *conn ) { + if (InterlockedDecrement( &conn->refs )) return; + TRACE( "Closing connection %p.\n", conn ); if (conn->secure) { free( conn->peek_msg_mem ); diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index e046b05581c..0f00cecedc6 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -1449,7 +1449,7 @@ static void CALLBACK connection_collector( TP_CALLBACK_INSTANCE *instance, void { TRACE("freeing %p\n", netconn); list_remove(&netconn->entry); - netconn_close(netconn); + netconn_release(netconn); } else remaining_connections++; } @@ -1591,7 +1591,7 @@ static DWORD open_connection( struct request *request ) if (netconn_is_alive( netconn )) break; TRACE("connection %p no longer alive, closing\n", netconn); - netconn_close( netconn ); + netconn_release( netconn ); netconn = NULL; } @@ -1654,7 +1654,7 @@ static DWORD open_connection( struct request *request ) { request->netconn = NULL; free( addressW ); - netconn_close( netconn ); + netconn_release( netconn ); return ret; } } @@ -1668,7 +1668,7 @@ static DWORD open_connection( struct request *request ) { request->netconn = NULL; free( addressW ); - netconn_close( netconn ); + netconn_release( netconn ); return ret; } } @@ -1687,7 +1687,7 @@ static DWORD open_connection( struct request *request ) if (netconn->secure && !(request->server_cert = netconn_get_certificate( netconn ))) { free( addressW ); - netconn_close( netconn ); + netconn_release( netconn ); return ERROR_WINHTTP_SECURE_FAILURE; } @@ -1705,7 +1705,7 @@ void close_connection( struct request *request ) if (!request->netconn) return; send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, 0, 0 ); - netconn_close( request->netconn ); + netconn_release( request->netconn ); request->netconn = NULL; send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, 0, 0 ); } @@ -1884,7 +1884,7 @@ static void finished_reading( struct request *request ) else if (!wcscmp( request->version, L"HTTP/1.0" )) close = TRUE; if (close) - netconn_close( request->netconn ); + netconn_release( request->netconn ); else cache_connection( request->netconn ); request->netconn = NULL; @@ -2715,7 +2715,7 @@ static DWORD handle_redirect( struct request *request, DWORD status ) goto end; } - netconn_close( request->netconn ); + netconn_release( request->netconn ); request->netconn = NULL; request->content_length = request->content_read = 0; request->read_pos = request->read_size = 0; diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index 53947f73351..2a38d1fb372 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -106,6 +106,7 @@ struct connect struct netconn { struct list entry; + LONG refs; int socket; struct sockaddr_storage sockaddr; BOOL secure; /* SSL active on connection? */ @@ -370,7 +371,7 @@ void close_connection( struct request * ) DECLSPEC_HIDDEN; void init_queue( struct queue *queue ) DECLSPEC_HIDDEN; void stop_queue( struct queue * ) DECLSPEC_HIDDEN; -void netconn_close( struct netconn * ) DECLSPEC_HIDDEN; +void netconn_release( struct netconn * ) DECLSPEC_HIDDEN; DWORD netconn_create( struct hostdata *, const struct sockaddr_storage *, int, struct netconn ** ) DECLSPEC_HIDDEN; void netconn_unload( void ) DECLSPEC_HIDDEN; ULONG netconn_query_data_available( struct netconn * ) DECLSPEC_HIDDEN; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1422
From: Paul Gofman <pgofman(a)codeweavers.com> The connection may be cached and checked for availability for request before the server thread closes it. --- dlls/winhttp/tests/notification.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/dlls/winhttp/tests/notification.c b/dlls/winhttp/tests/notification.c index cab297d3f4c..48b0b77ca7c 100644 --- a/dlls/winhttp/tests/notification.c +++ b/dlls/winhttp/tests/notification.c @@ -1401,7 +1401,7 @@ struct server_info }; static int server_socket; -static HANDLE server_socket_available, server_socket_done; +static HANDLE server_socket_available, server_socket_closed, server_socket_done; static DWORD CALLBACK server_thread(LPVOID param) { @@ -1436,7 +1436,7 @@ static DWORD CALLBACK server_thread(LPVOID param) do { if (c == -1) c = accept(s, NULL, NULL); - + ResetEvent(server_socket_closed); memset(buffer, 0, sizeof buffer); for(i = 0; i < sizeof buffer - 1; i++) { @@ -1463,6 +1463,7 @@ static DWORD CALLBACK server_thread(LPVOID param) } shutdown(c, 2); closesocket(c); + SetEvent(server_socket_closed); c = -1; } while (!last_request); @@ -1749,6 +1750,7 @@ static void test_persistent_connection(int port) SetEvent( server_socket_done ); CloseHandle( info.wait ); + WaitForSingleObject( server_socket_closed, INFINITE ); } struct test_recursion_context @@ -1929,6 +1931,7 @@ START_TEST (notification) ok( thread != NULL, "failed to create thread %lu\n", GetLastError() ); server_socket_available = CreateEventW( NULL, 0, 0, NULL ); + server_socket_closed = CreateEventW( NULL, 0, 0, NULL ); server_socket_done = CreateEventW( NULL, 0, 0, NULL ); ret = WaitForSingleObject( si.event, 10000 ); @@ -1948,4 +1951,5 @@ START_TEST (notification) CloseHandle( thread ); CloseHandle( server_socket_available ); CloseHandle( server_socket_done ); + CloseHandle( server_socket_closed ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1422
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 0f00cecedc6..a51581a3f93 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -1876,8 +1876,9 @@ static void finished_reading( struct request *request ) if (request->netconn->socket == -1) close = TRUE; else if (request->hdr.disable_flags & WINHTTP_DISABLE_KEEP_ALIVE) close = TRUE; - else if (!query_headers( request, WINHTTP_QUERY_CONNECTION, NULL, connection, &size, NULL ) || - !query_headers( request, WINHTTP_QUERY_PROXY_CONNECTION, NULL, connection, &size, NULL )) + else if (!query_headers( request, WINHTTP_QUERY_CONNECTION | WINHTTP_QUERY_FLAG_REQUEST_HEADERS, + NULL, connection, &size, NULL ) || !query_headers( request, WINHTTP_QUERY_PROXY_CONNECTION + | WINHTTP_QUERY_FLAG_REQUEST_HEADERS, NULL, connection, &size, NULL )) { if (!wcsicmp( connection, L"close" )) close = TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1422
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 6 ++++-- dlls/winhttp/tests/notification.c | 28 ++++++---------------------- 2 files changed, 10 insertions(+), 24 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index a51581a3f93..2b7698da65b 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -1704,10 +1704,8 @@ void close_connection( struct request *request ) { if (!request->netconn) return; - send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, 0, 0 ); netconn_release( request->netconn ); request->netconn = NULL; - send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, 0, 0 ); } static DWORD add_host_header( struct request *request, DWORD modifier ) @@ -1885,7 +1883,11 @@ static void finished_reading( struct request *request ) else if (!wcscmp( request->version, L"HTTP/1.0" )) close = TRUE; if (close) + { + send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, 0, 0 ); netconn_release( request->netconn ); + send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, 0, 0 ); + } else cache_connection( request->netconn ); request->netconn = NULL; diff --git a/dlls/winhttp/tests/notification.c b/dlls/winhttp/tests/notification.c index 48b0b77ca7c..181bd35856b 100644 --- a/dlls/winhttp/tests/notification.c +++ b/dlls/winhttp/tests/notification.c @@ -146,8 +146,6 @@ static const struct notification cache_test[] = { winhttp_send_request, WINHTTP_CALLBACK_STATUS_REQUEST_SENT }, { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE }, { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RESPONSE_RECEIVED }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_WINE_ALLOW }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, NF_WINE_ALLOW }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL }, { winhttp_open_request, WINHTTP_CALLBACK_STATUS_HANDLE_CREATED }, { winhttp_send_request, WINHTTP_CALLBACK_STATUS_CONNECTING_TO_SERVER, NF_WINE_ALLOW }, @@ -156,8 +154,6 @@ static const struct notification cache_test[] = { winhttp_send_request, WINHTTP_CALLBACK_STATUS_REQUEST_SENT }, { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE }, { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RESPONSE_RECEIVED }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_WINE_ALLOW }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, NF_WINE_ALLOW }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL }, @@ -171,8 +167,6 @@ static const struct notification cache_test[] = { winhttp_send_request, WINHTTP_CALLBACK_STATUS_REQUEST_SENT }, { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE }, { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RESPONSE_RECEIVED }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_WINE_ALLOW }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, NF_WINE_ALLOW }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL }, { winhttp_open_request, WINHTTP_CALLBACK_STATUS_HANDLE_CREATED }, { winhttp_send_request, WINHTTP_CALLBACK_STATUS_CONNECTING_TO_SERVER, NF_WINE_ALLOW }, @@ -181,8 +175,6 @@ static const struct notification cache_test[] = { winhttp_send_request, WINHTTP_CALLBACK_STATUS_REQUEST_SENT }, { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE }, { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RESPONSE_RECEIVED }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_WINE_ALLOW }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, NF_WINE_ALLOW }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL } @@ -442,8 +434,6 @@ static const struct notification redirect_test[] = { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_REQUEST_SENT }, { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE }, { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RESPONSE_RECEIVED }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_WINE_ALLOW }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, NF_WINE_ALLOW }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL } @@ -523,6 +513,8 @@ static const struct notification async_test[] = { winhttp_query_data, WINHTTP_CALLBACK_STATUS_DATA_AVAILABLE, NF_SIGNAL }, { winhttp_read_data, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE, NF_ALLOW }, { winhttp_read_data, WINHTTP_CALLBACK_STATUS_RESPONSE_RECEIVED, NF_ALLOW }, + { winhttp_read_data, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION }, + { winhttp_read_data, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED }, { winhttp_read_data, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, NF_SIGNAL }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, @@ -578,6 +570,10 @@ static void test_async( void ) ok( req != NULL, "failed to open a request %lu\n", err ); ok( err == ERROR_SUCCESS, "got %lu\n", err ); + ret = WinHttpAddRequestHeaders( req , L"Connection: close", -1L, WINHTTP_ADDREQ_FLAG_REPLACE | WINHTTP_ADDREQ_FLAG_ADD ); + err = GetLastError(); + ok(ret, "WinHttpAddRequestHeaders failed to add new header, got %d with error %lu\n", ret, err); + setup_test( &info, winhttp_send_request, __LINE__ ); SetLastError( 0xdeadbeef ); ret = WinHttpSendRequest( req, NULL, 0, NULL, 0, 0, 0 ); @@ -685,8 +681,6 @@ static const struct notification websocket_test[] = { winhttp_websocket_receive, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, NF_SAVE_BUFFER | NF_SIGNAL }, { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_CLOSE_COMPLETE, NF_SIGNAL }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_WINE_ALLOW }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, NF_WINE_ALLOW }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL }, }; @@ -708,8 +702,6 @@ static const struct notification websocket_test2[] = { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, NF_MAIN_THREAD | NF_SAVE_BUFFER}, { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_CLOSE_COMPLETE, NF_SIGNAL }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_WINE_ALLOW }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, NF_WINE_ALLOW }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL } }; @@ -733,8 +725,6 @@ static const struct notification websocket_test3[] = { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, NF_SAVE_BUFFER }, { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_WINE_ALLOW }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, NF_WINE_ALLOW }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL }, }; @@ -757,8 +747,6 @@ static struct notification websocket_test4[] = { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, NF_SAVE_BUFFER }, { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_WINE_ALLOW }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, NF_WINE_ALLOW }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL }, }; @@ -783,8 +771,6 @@ static const struct notification websocket_test5[] = { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_CLOSE_COMPLETE, NF_MAIN_THREAD| NF_SAVE_BUFFER | NF_SIGNAL }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_WINE_ALLOW }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, NF_WINE_ALLOW }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL } @@ -1626,8 +1612,6 @@ static const struct notification close_request_test[] = static const struct notification close_allow_connection_close_request_test[] = { - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_ALLOW }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, NF_ALLOW }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1422
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/net.c | 5 +++++ dlls/winhttp/request.c | 15 +++++++++------ dlls/winhttp/winhttp_private.h | 2 ++ 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/dlls/winhttp/net.c b/dlls/winhttp/net.c index 0652dfcb30a..8c7dcdb7eca 100644 --- a/dlls/winhttp/net.c +++ b/dlls/winhttp/net.c @@ -278,6 +278,11 @@ DWORD netconn_create( struct hostdata *host, const struct sockaddr_storage *sock return ERROR_SUCCESS; } +void netconn_addref( struct netconn *conn ) +{ + InterlockedIncrement( &conn->refs ); +} + void netconn_release( struct netconn *conn ) { if (InterlockedDecrement( &conn->refs )) return; diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 2b7698da65b..d2dfc0967f0 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -3158,7 +3158,7 @@ static void socket_handle_closing( struct object_header *hdr ) pending_tasks = cancel_queue( &socket->recv_q ) || pending_tasks; if (pending_tasks) - netconn_cancel_io( socket->request->netconn ); + netconn_cancel_io( socket->netconn ); } static BOOL socket_query_option( struct object_header *hdr, DWORD option, void *buffer, DWORD *buflen ) @@ -3184,6 +3184,7 @@ static void socket_destroy( struct object_header *hdr ) stop_queue( &socket->send_q ); stop_queue( &socket->recv_q ); + netconn_release( socket->netconn ); release_object( &socket->request->hdr ); free( socket->send_frame_buffer ); free( socket ); @@ -3206,7 +3207,7 @@ static BOOL socket_set_option( struct object_header *hdr, DWORD option, void *bu return FALSE; } socket->keepalive_interval = interval; - netconn_set_timeout( socket->request->netconn, FALSE, socket->keepalive_interval ); + netconn_set_timeout( socket->netconn, FALSE, socket->keepalive_interval ); SetLastError( ERROR_SUCCESS ); TRACE( "WINHTTP_OPTION_WEB_SOCKET_KEEPALIVE_INTERVAL %lu.\n", interval); return TRUE; @@ -3261,11 +3262,13 @@ HINTERNET WINAPI WinHttpWebSocketCompleteUpgrade( HINTERNET hrequest, DWORD_PTR InitializeSRWLock( &socket->send_lock ); init_queue( &socket->send_q ); init_queue( &socket->recv_q ); + netconn_addref( request->netconn ); + socket->netconn = request->netconn; addref_object( &request->hdr ); socket->request = request; - netconn_set_timeout( socket->request->netconn, FALSE, socket->keepalive_interval ); + netconn_set_timeout( socket->netconn, FALSE, socket->keepalive_interval ); if ((hsocket = alloc_handle( &socket->hdr ))) { @@ -3283,7 +3286,7 @@ static DWORD send_bytes( struct socket *socket, char *bytes, int len, int *sent, { int count; DWORD err; - err = netconn_send( socket->request->netconn, bytes, len, &count, ovr ); + err = netconn_send( socket->netconn, bytes, len, &count, ovr ); if (sent) *sent = count; if (err) return err; return (count == len || (ovr && count)) ? ERROR_SUCCESS : ERROR_INTERNAL_ERROR; @@ -3395,7 +3398,7 @@ static DWORD complete_send_frame( struct socket *socket, WSAOVERLAPPED *ovr, con { DWORD ret, len, i; - if (!netconn_wait_overlapped_result( socket->request->netconn, ovr, &len )) + if (!netconn_wait_overlapped_result( socket->netconn, ovr, &len )) return WSAGetLastError(); if (socket->bytes_in_send_frame_buffer) @@ -3674,7 +3677,7 @@ static DWORD receive_bytes( struct socket *socket, char *buf, DWORD len, DWORD * } while (size != len) { - if ((err = netconn_recv( socket->request->netconn, ptr, needed, 0, &received ))) return err; + if ((err = netconn_recv( socket->netconn, ptr, needed, 0, &received ))) return err; if (!received) break; size += received; if (!read_full_buffer) break; diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index 2a38d1fb372..996b61ac63e 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -257,6 +257,7 @@ struct socket { struct object_header hdr; struct request *request; + struct netconn *netconn; int keepalive_interval; unsigned int send_buffer_size; enum socket_state state; @@ -371,6 +372,7 @@ void close_connection( struct request * ) DECLSPEC_HIDDEN; void init_queue( struct queue *queue ) DECLSPEC_HIDDEN; void stop_queue( struct queue * ) DECLSPEC_HIDDEN; +void netconn_addref( struct netconn * ) DECLSPEC_HIDDEN; void netconn_release( struct netconn * ) DECLSPEC_HIDDEN; DWORD netconn_create( struct hostdata *, const struct sockaddr_storage *, int, struct netconn ** ) DECLSPEC_HIDDEN; void netconn_unload( void ) DECLSPEC_HIDDEN; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1422
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index d2dfc0967f0..33a04829404 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -3257,6 +3257,7 @@ HINTERNET WINAPI WinHttpWebSocketCompleteUpgrade( HINTERNET hrequest, DWORD_PTR socket->hdr.callback = request->hdr.callback; socket->hdr.notify_mask = request->hdr.notify_mask; socket->hdr.context = context; + socket->hdr.flags = request->connect->hdr.flags & WINHTTP_FLAG_ASYNC; socket->keepalive_interval = 30000; socket->send_buffer_size = request->websocket_send_buffer_size; InitializeSRWLock( &socket->send_lock ); @@ -3581,7 +3582,7 @@ DWORD WINAPI WinHttpWebSocketSend( HINTERNET hsocket, WINHTTP_WEB_SOCKET_BUFFER_ return ERROR_INVALID_OPERATION; } - if (socket->request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) + if (socket->hdr.flags & WINHTTP_FLAG_ASYNC) { BOOL async_send, complete_async = FALSE; struct socket_send *s; @@ -3756,7 +3757,7 @@ static void task_socket_send_pong( void *ctx, BOOL abort ) static DWORD socket_send_pong( struct socket *socket ) { - if (socket->request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) + if (socket->hdr.flags & WINHTTP_FLAG_ASYNC) { BOOL async_send, complete_async = FALSE; struct socket_send *s; @@ -4018,7 +4019,7 @@ DWORD WINAPI WinHttpWebSocketReceive( HINTERNET hsocket, void *buf, DWORD len, D return ERROR_INVALID_OPERATION; } - if (socket->request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) + if (socket->hdr.flags & WINHTTP_FLAG_ASYNC) { struct socket_receive *r; @@ -4088,7 +4089,7 @@ static DWORD send_socket_shutdown( struct socket *socket, USHORT status, const v if (socket->state < SOCKET_STATE_SHUTDOWN) socket->state = SOCKET_STATE_SHUTDOWN; - if (socket->request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) + if (socket->hdr.flags & WINHTTP_FLAG_ASYNC) { BOOL async_send, complete_async = FALSE; struct socket_shutdown *s; @@ -4246,7 +4247,7 @@ DWORD WINAPI WinHttpWebSocketClose( HINTERNET hsocket, USHORT status, void *reas prev_state = socket->state; socket->state = SOCKET_STATE_CLOSED; - if (socket->request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) + if (socket->hdr.flags & WINHTTP_FLAG_ASYNC) { pending_receives = InterlockedIncrement( &socket->hdr.pending_receives ); cancel_queue( &socket->recv_q ); @@ -4257,12 +4258,12 @@ DWORD WINAPI WinHttpWebSocketClose( HINTERNET hsocket, USHORT status, void *reas if (pending_receives == 1 && socket->close_frame_received) { - if (socket->request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) + if (socket->hdr.flags & WINHTTP_FLAG_ASYNC) socket_close_complete( socket, socket->close_frame_receive_err ); goto done; } - if (socket->request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) + if (socket->hdr.flags & WINHTTP_FLAG_ASYNC) { struct socket_shutdown *s; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1422
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 18 ++++++++++++++---- dlls/winhttp/winhttp_private.h | 2 ++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 33a04829404..a3420507da7 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -3186,6 +3186,7 @@ static void socket_destroy( struct object_header *hdr ) netconn_release( socket->netconn ); release_object( &socket->request->hdr ); + free( socket->read_buffer ); free( socket->send_frame_buffer ); free( socket ); } @@ -3266,6 +3267,14 @@ HINTERNET WINAPI WinHttpWebSocketCompleteUpgrade( HINTERNET hrequest, DWORD_PTR netconn_addref( request->netconn ); socket->netconn = request->netconn; + if (request->read_size) + { + socket->read_buffer = malloc( request->read_size ); + socket->bytes_in_read_buffer = request->read_size; + memcpy( socket->read_buffer, request->read_buf + request->read_pos, request->read_size ); + request->read_pos = request->read_size = 0; + } + addref_object( &request->hdr ); socket->request = request; @@ -3668,11 +3677,12 @@ static DWORD receive_bytes( struct socket *socket, char *buf, DWORD len, DWORD * char *ptr = buf; int received; - if (socket->request->read_size) + if (socket->bytes_in_read_buffer) { - size = min( needed, socket->request->read_size ); - memcpy( ptr, socket->request->read_buf + socket->request->read_pos, size ); - remove_data( socket->request, size ); + size = min( needed, socket->bytes_in_read_buffer ); + memcpy( ptr, socket->read_buffer, size ); + memmove( socket->read_buffer, socket->read_buffer + size, socket->bytes_in_read_buffer - size ); + socket->bytes_in_read_buffer -= size; needed -= size; ptr += size; } diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index 996b61ac63e..5885f46058d 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -277,6 +277,8 @@ struct socket unsigned int send_remaining_size; unsigned int bytes_in_send_frame_buffer; unsigned int client_buffer_offset; + char *read_buffer; + unsigned int bytes_in_read_buffer; SRWLOCK send_lock; volatile LONG pending_noncontrol_send; enum fragment_type sending_fragment_type; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1422
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 4 ---- dlls/winhttp/tests/notification.c | 8 +++++--- dlls/winhttp/winhttp_private.h | 1 - 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index a3420507da7..15123d8bff1 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -3185,7 +3185,6 @@ static void socket_destroy( struct object_header *hdr ) stop_queue( &socket->recv_q ); netconn_release( socket->netconn ); - release_object( &socket->request->hdr ); free( socket->read_buffer ); free( socket->send_frame_buffer ); free( socket ); @@ -3275,9 +3274,6 @@ HINTERNET WINAPI WinHttpWebSocketCompleteUpgrade( HINTERNET hrequest, DWORD_PTR request->read_pos = request->read_size = 0; } - addref_object( &request->hdr ); - socket->request = request; - netconn_set_timeout( socket->netconn, FALSE, socket->keepalive_interval ); if ((hsocket = alloc_handle( &socket->hdr ))) diff --git a/dlls/winhttp/tests/notification.c b/dlls/winhttp/tests/notification.c index 181bd35856b..a9f782f5e3d 100644 --- a/dlls/winhttp/tests/notification.c +++ b/dlls/winhttp/tests/notification.c @@ -671,7 +671,8 @@ static const struct notification websocket_test[] = { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE }, { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RESPONSE_RECEIVED }, { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_HEADERS_AVAILABLE, NF_SIGNAL }, - { winhttp_websocket_complete_upgrade, WINHTTP_CALLBACK_STATUS_HANDLE_CREATED, NF_SIGNAL }, + { winhttp_websocket_complete_upgrade, WINHTTP_CALLBACK_STATUS_HANDLE_CREATED }, + { winhttp_websocket_complete_upgrade, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL }, { winhttp_websocket_send, WINHTTP_CALLBACK_STATUS_WRITE_COMPLETE, NF_MAIN_THREAD | NF_SIGNAL }, { winhttp_websocket_send, WINHTTP_CALLBACK_STATUS_WRITE_COMPLETE, NF_MAIN_THREAD | NF_SIGNAL }, { winhttp_websocket_send, WINHTTP_CALLBACK_STATUS_WRITE_COMPLETE, NF_MAIN_THREAD | NF_SIGNAL }, @@ -680,7 +681,6 @@ static const struct notification websocket_test[] = { winhttp_websocket_receive, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, NF_SAVE_BUFFER | NF_SIGNAL }, { winhttp_websocket_receive, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, NF_SAVE_BUFFER | NF_SIGNAL }, { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_CLOSE_COMPLETE, NF_SIGNAL }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL }, }; @@ -911,6 +911,9 @@ static void test_websocket(BOOL secure) err = GetLastError(); ok( socket != NULL, "got %lu\n", err ); ok( err == ERROR_SUCCESS, "got %lu\n", err ); + + WinHttpCloseHandle( request ); + WaitForSingleObject( info.wait, INFINITE ); /* The send is executed synchronously (even if sending a reasonably big buffer exceeding SSL buffer size). @@ -1029,7 +1032,6 @@ static void test_websocket(BOOL secure) setup_test( &info, winhttp_close_handle, __LINE__ ); WinHttpCloseHandle( socket ); - WinHttpCloseHandle( request ); WaitForSingleObject( info.wait, INFINITE ); end_test( &info, __LINE__ ); diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index 5885f46058d..4e9c14a2ed7 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -256,7 +256,6 @@ enum fragment_type struct socket { struct object_header hdr; - struct request *request; struct netconn *netconn; int keepalive_interval; unsigned int send_buffer_size; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1422
Hans Leidekker (@hans) commented about dlls/winhttp/request.c:
if (request->netconn->socket == -1) close = TRUE; else if (request->hdr.disable_flags & WINHTTP_DISABLE_KEEP_ALIVE) close = TRUE; - else if (!query_headers( request, WINHTTP_QUERY_CONNECTION, NULL, connection, &size, NULL ) || - !query_headers( request, WINHTTP_QUERY_PROXY_CONNECTION, NULL, connection, &size, NULL )) + else if (!query_headers( request, WINHTTP_QUERY_CONNECTION | WINHTTP_QUERY_FLAG_REQUEST_HEADERS, + NULL, connection, &size, NULL ) || !query_headers( request, WINHTTP_QUERY_PROXY_CONNECTION + | WINHTTP_QUERY_FLAG_REQUEST_HEADERS, NULL, connection, &size, NULL ))
Should we keep checking the response headers? It doesn't seem useful to cache a connection that the server is going to close. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1422#note_17311
Hans Leidekker (@hans) commented about dlls/winhttp/request.c:
netconn_addref( request->netconn ); socket->netconn = request->netconn;
+ if (request->read_size) + { + socket->read_buffer = malloc( request->read_size ); + socket->bytes_in_read_buffer = request->read_size; + memcpy( socket->read_buffer, request->read_buf + request->read_pos, request->read_size ); + request->read_pos = request->read_size = 0; + }
Please check for allocation failure. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1422#note_17312
participants (3)
-
Hans Leidekker (＠hans) -
Paul Gofman
-
Paul Gofman (＠gofman)