[PATCH v2 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).
-- v2: winhttp: Only read server reply in send_request() if the whole request is available.
From: Paul Gofman pgofman@codeweavers.com
--- dlls/winhttp/request.c | 49 +++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 24 deletions(-)
diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index c99cb7ee195..5af2609158a 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) + switch (request->state) { - ret = ERROR_WINHTTP_INCORRECT_HANDLE_STATE; - goto done; - } + 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; + send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE, NULL, 0 ); + break;
- if (request->state == 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; - } + 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 );
- 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 ); + case REQUEST_RESPONSE_STATE_REQUEST_SENT: + send_callback( &request->hdr, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE, NULL, 0 ); + break;
- if (return_to_send) - { - request->state = REQUEST_RESPONSE_STATE_READ_RESPONSE_QUEUED; - return queue_receive_response( request ); + 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,
From: Paul Gofman pgofman@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 5af2609158a..38ffa632f16 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, };
v2: - also mind Content-length request header which may be set by an app and takes precedence over total length WinHttpSendRequest parameter.
Hans Leidekker (@hans) commented about dlls/winhttp/request.c:
{ 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)
- switch (request->state) {
ret = ERROR_WINHTTP_INCORRECT_HANDLE_STATE;goto done;- }
case REQUEST_RESPONSE_RECURSIVE_REQUEST:
Could you remove one level of indentation in the switch?
Hans Leidekker (@hans) commented about dlls/winhttp/winhttp_private.h:
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,
"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.
-
Hans Leidekker (＠hans) -
Paul Gofman
-
Paul Gofman (＠gofman)