[PATCH v3 0/2] MR1851: winhttp: Only read server reply in send_request() if the whole request is available.
Fixes a regression introduced by aa8f97e8294b0618de0e8a45ff2adef0c4e516da. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54184 For the requests which involve additional data to be sent the application might use WinHttpWriteData() before WinHttpReceiveResponse() to pass the additional request data instead of providing the optional data to WinHttpSendRequest(). In that case server reply can't be read in send_request(). I believe the first patch straightens up state handling in receive_response() by introducing a single switch processing every incoming state and avoiding additional local variable (along with making WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE notification handling less obscure). -- v3: winhttp: Only read server reply in send_request() if the whole request is sent. winhttp: Handle state in a single switch in receive_response(). https://gitlab.winehq.org/wine/wine/-/merge_requests/1851
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 45 +++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index c99cb7ee195..43d123de129 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -2871,38 +2871,39 @@ static DWORD receive_response( struct request *request ) { BOOL async_mode = request->connect->hdr.flags & WINHTTP_FLAG_ASYNC; DWORD ret, size, query, status; - BOOL return_to_send; TRACE( "request state %d.\n", request->state ); - if (request->state >= REQUEST_RESPONSE_STATE_RESPONSE_RECEIVED) - { - ret = ERROR_WINHTTP_INCORRECT_HANDLE_STATE; - goto done; - } - - if (request->state == REQUEST_RESPONSE_RECURSIVE_REQUEST) + switch (request->state) { + case REQUEST_RESPONSE_RECURSIVE_REQUEST: TRACE( "Sending request.\n" ); if ((ret = send_request( request, NULL, 0, request->optional, request->optional_len, 0, 0, FALSE ))) goto done; - } - - return_to_send = async_mode && request->state == REQUEST_RESPONSE_STATE_SENDING_REQUEST; - if (request->state < REQUEST_RESPONSE_STATE_REQUEST_SENT && !return_to_send) - { - ret = ERROR_WINHTTP_INCORRECT_HANDLE_STATE; - goto done; - } - - if (request->state == REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED_REQUEST_SENT) - request->state = REQUEST_RESPONSE_STATE_REQUEST_SENT; - else send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE, NULL, 0 ); + break; - if (return_to_send) - { + case REQUEST_RESPONSE_STATE_SENDING_REQUEST: + if (!async_mode) + { + ret = ERROR_WINHTTP_INCORRECT_HANDLE_STATE; + goto done; + } + send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE, NULL, 0 ); request->state = REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED; return queue_receive_response( request ); + + + case REQUEST_RESPONSE_STATE_REQUEST_SENT: + send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE, NULL, 0 ); + break; + + case REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED_REQUEST_SENT: + request->state = REQUEST_RESPONSE_STATE_REQUEST_SENT; + break; + + default: + ret = ERROR_WINHTTP_INCORRECT_HANDLE_STATE; + goto done; } send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_RESPONSE_RECEIVED, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1851
From: Paul Gofman <pgofman(a)codeweavers.com> Fixes a regression introduced by aa8f97e8294b0618de0e8a45ff2adef0c4e516da. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54184 --- dlls/winhttp/request.c | 45 +++++++++++++++++++++++++------ dlls/winhttp/tests/notification.c | 36 ++++++++++++++++++------- dlls/winhttp/winhttp_private.h | 2 ++ 3 files changed, 66 insertions(+), 17 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 43d123de129..07d0ff19b7c 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -2216,9 +2216,9 @@ static DWORD send_request( struct request *request, const WCHAR *headers, DWORD { struct connect *connect = request->connect; struct session *session = connect->session; + DWORD ret, len, buflen, content_length; char *wire_req; int bytes_sent; - DWORD ret, len; TRACE( "request state %d.\n", request->state ); @@ -2305,14 +2305,29 @@ static DWORD send_request( struct request *request, const WCHAR *headers, DWORD request->optional_len = optional_len; len += optional_len; } - netconn_set_timeout( request->netconn, FALSE, request->receive_response_timeout ); - request->read_reply_status = read_reply( request ); send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_REQUEST_SENT, &len, sizeof(len) ); - if (request->state == REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED) - request->state = REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED_REQUEST_SENT; + buflen = sizeof(content_length); + if (query_headers( request, WINHTTP_QUERY_FLAG_REQUEST_HEADERS | WINHTTP_QUERY_CONTENT_LENGTH + | WINHTTP_QUERY_FLAG_NUMBER, NULL, &content_length, &buflen, NULL )) + content_length = total_len; + + if (content_length <= optional_len) + { + netconn_set_timeout( request->netconn, FALSE, request->receive_response_timeout ); + request->read_reply_status = read_reply( request ); + if (request->state == REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED) + request->state = REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED_REPLY_RECEIVED; + else + request->state = REQUEST_RESPONSE_STATE_REPLY_RECEIVED; + } else - request->state = REQUEST_RESPONSE_STATE_REQUEST_SENT; + { + if (request->state == REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED) + request->state = REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED_REQUEST_SENT; + else + request->state = REQUEST_RESPONSE_STATE_REQUEST_SENT; + } end: if (async) @@ -2895,10 +2910,24 @@ static DWORD receive_response( struct request *request ) case REQUEST_RESPONSE_STATE_REQUEST_SENT: send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE, NULL, 0 ); + if (async_mode) + { + request->state = REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED_REQUEST_SENT; + return queue_receive_response( request ); + } + /* fallthrough */ + case REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED_REQUEST_SENT: + netconn_set_timeout( request->netconn, FALSE, request->receive_response_timeout ); + request->read_reply_status = read_reply( request ); + request->state = REQUEST_RESPONSE_STATE_REPLY_RECEIVED; break; - case REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED_REQUEST_SENT: - request->state = REQUEST_RESPONSE_STATE_REQUEST_SENT; + case REQUEST_RESPONSE_STATE_REPLY_RECEIVED: + send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE, NULL, 0 ); + break; + + case REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED_REPLY_RECEIVED: + request->state = REQUEST_RESPONSE_STATE_REPLY_RECEIVED; break; default: diff --git a/dlls/winhttp/tests/notification.c b/dlls/winhttp/tests/notification.c index dbdb958f061..78319f0df5c 100644 --- a/dlls/winhttp/tests/notification.c +++ b/dlls/winhttp/tests/notification.c @@ -1852,6 +1852,8 @@ struct test_recursion_context DWORD main_thread_id; DWORD receive_response_thread_id; BOOL headers_available; + DWORD total_len; + BYTE *send_buffer; }; /* The limit is 128 before Win7 and 3 on newer Windows. */ @@ -1872,17 +1874,29 @@ static void CALLBACK test_recursion_callback( HINTERNET handle, DWORD_PTR contex case WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE: if (status == context->call_receive_response_status) { - context->receive_response_thread_id = GetCurrentThreadId(); - ret = WinHttpReceiveResponse( context->request, NULL ); - ok( ret, "failed to receive response, GetLastError() %lu\n", GetLastError() ); + if (context->total_len) + { + ret = WinHttpWriteData( context->request, context->send_buffer, context->total_len, NULL ); + ok(ret, "failed.\n"); + } + else + { + context->receive_response_thread_id = GetCurrentThreadId(); + ret = WinHttpReceiveResponse( context->request, NULL ); + ok( ret, "failed to receive response, GetLastError() %lu\n", GetLastError() ); + } } break; + case WINHTTP_CALLBACK_STATUS_WRITE_COMPLETE: + trace("WINHTTP_CALLBACK_STATUS_WRITE_COMPLETE thread %04lx.\n", GetCurrentThreadId()); + context->receive_response_thread_id = GetCurrentThreadId(); + ret = WinHttpReceiveResponse( context->request, NULL ); + ok( ret, "failed to receive response, GetLastError() %lu\n", GetLastError() ); + break; + case WINHTTP_CALLBACK_STATUS_HEADERS_AVAILABLE: - if (context->call_receive_response_status == WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE) - ok( GetCurrentThreadId() == context->receive_response_thread_id, - "expected callback to be called from the same thread, got %lx.\n", GetCurrentThreadId() ); - else + if (context->call_receive_response_status != WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE) ok( GetCurrentThreadId() != context->main_thread_id, "expected callback to be called from the other thread, got main.\n" ); context->headers_available = TRUE; @@ -1933,7 +1947,6 @@ static void CALLBACK test_recursion_callback( HINTERNET handle, DWORD_PTR contex InterlockedDecrement( &context->recursion_count ); break; case WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE: - case WINHTTP_CALLBACK_STATUS_RESPONSE_RECEIVED: if (!context->headers_available && context->call_receive_response_status == WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE) ok( GetCurrentThreadId() == context->receive_response_thread_id, @@ -1979,7 +1992,10 @@ static void test_recursion(void) ok( ret, "failed to receive response, GetLastError() %lu\n", GetLastError() ); context.call_receive_response_status = WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE; - ret = WinHttpSendRequest( request, NULL, 0, NULL, 0, 0, (DWORD_PTR)&context ); + context.total_len = 1; + context.send_buffer = &b; + b = 0; + ret = WinHttpSendRequest( request, NULL, 0, NULL, 0, context.total_len, (DWORD_PTR)&context ); err = GetLastError(); if (!ret && (err == ERROR_WINHTTP_CANNOT_CONNECT || err == ERROR_WINHTTP_TIMEOUT)) { @@ -1994,6 +2010,8 @@ static void test_recursion(void) ok( ret, "failed to send request, GetLastError() %lu\n", GetLastError() ); WaitForSingleObject( context.wait, INFINITE ); + context.total_len = 0; + context.send_buffer = NULL; size = sizeof(status); ret = WinHttpQueryHeaders( request, WINHTTP_QUERY_STATUS_CODE | WINHTTP_QUERY_FLAG_NUMBER, NULL, diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index 8e4789356ec..bf1e60e2cee 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -181,6 +181,8 @@ enum request_response_state REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED, REQUEST_RESPONSE_STATE_REQUEST_SENT, REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED_REQUEST_SENT, + REQUEST_RESPONSE_STATE_REPLY_RECEIVED, + REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED_REPLY_RECEIVED, REQUEST_RESPONSE_RECURSIVE_REQUEST, REQUEST_RESPONSE_STATE_RESPONSE_RECEIVED, }; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1851
v3: - change indentation in switch; - rename patch 2. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1851#note_19931
On Tue Dec 20 09:31:09 2022 +0000, Hans Leidekker wrote:
"winhttp: Only read server reply in send_request() if the whole request is sent." would be a better title for this patch. It won't block this patch but I still think that the state machine is overly complex. I suspect that a model where we always queue but synchronize when the request is synchronous or when it is opportune would simplify this. I am afraid we can't plainly queue and wait in WinHttpReceiveResponse(), it will currently block if queued from async callback as we have one thread processing requests at a time. Or do you mean something else?
As I recently discovered however such a behaviour is not the case on Windows. I can queue request from async callback and wait for event to be signaled by its completion notification; that will go to another thread and won't deadlock (unlike our winhttp currently). I have not yet encountered an app depending on that. Yet, even if implementing that (which might be not entirely trivial as I suspect some synchronisation should still exist) we would be moving implementation farther away from Windows. I don't know any app currently depending on the request to be completed not just synchronous but on the same thread, but I keep encountering more and more patterns of winhttp async usage across the games. Might be only a matter of time until some game will depend not only on request completing synchoronously but also Tls variables being the same between the requester and notification. We also have a bit of validation missing across the functions (e. g., WinHttpWriteData) which I suppose will need the request state as well. Yet quite possible there is still some way to straighten this up which I don't yet see... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1851#note_19932
On Tue Dec 20 15:18:19 2022 +0000, Paul Gofman wrote:
I am afraid we can't plainly queue and wait in WinHttpReceiveResponse(), it will currently block if queued from async callback as we have one thread processing requests at a time. Or do you mean something else? As I recently discovered however such a behaviour is not the case on Windows. I can queue request from async callback and wait for event to be signaled by its completion notification; that will go to another thread and won't deadlock (unlike our winhttp currently). I have not yet encountered an app depending on that. Yet, even if implementing that (which might be not entirely trivial as I suspect some synchronisation should still exist) we would be moving implementation farther away from Windows. I don't know any app currently depending on the request to be completed not just synchronous but on the same thread, but I keep encountering more and more patterns of winhttp async usage across the games. Might be only a matter of time until some game will depend not only on request completing synchoronously but also Tls variables being the same between the requester and notification. We also have a bit of validation missing across the functions (e. g., WinHttpWriteData) which I suppose will need the request state as well. Yet quite possible there is still some way to straighten this up which I don't yet see... Not directly related, but now when stomping on those requests completed with WinHttpWriteData() I got curious how that is supposed to work with recursive requests and figured we are not handling it right (tested with redirect). We will currently send recursed request without body if it is not provided in optional data. Windows in this case returns ERROR_WINHTTP_RESEND_REQUEST and then resending the request and resupplying the data with WinHttpWriteData() lets it complete correctly. I have a patch for that but that is pre-existing issue and not a regression, so probably for after code freeze.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1851#note_19933
I am afraid we can't plainly queue and wait in WinHttpReceiveResponse(), it will currently block if queued from async callback as we have one thread processing requests at a time. Or do you mean something else?
Right, that means we will have to allow more than one thread processing requests. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1851#note_19950
This merge request was approved by Hans Leidekker. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1851
participants (3)
-
Hans Leidekker (＠hans) -
Paul Gofman
-
Paul Gofman (＠gofman)