[PATCH 0/3] MR5975: winhttp: Skip trailing spaces in reply headers.

[Paul Gofman (＠gofman)]

Fixes Ball at Work: The Ultimate Speedrun Platformer! failing to start (which receives such non-standard headers from the servers).

While testing this I found more weird aspects of handling invalid reply header names on Windows:
- leading spaces are not skipped, but if the first character is a space then two more spaces added at start of header name and then all the other invalid characters in the name (like '@' or 0xfe) are ignored (otherwise without the space at name start the presence of such header fails request); then such a header saved by failing requests can't be queried with WinHttpQueryHeaders() as the presence of invalid characters (including space) fails that with ERROR_INVALID_PARAMETER;
- trailing '\\t' are not skipped but also do not fail the request and the header with tab is present in raw headers.

But I suppose we don't need to complicate things by replicating this behaviour precisely until anything depends on that. The specific game motivating the patch only receives spaces in the end of header name which are skipped on Windows while this behaviour has some obvious logic (even if doesn't follow http standard).

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5975

[PATCH 1/3] winhttp/tests: Add test for trailing spaces in reply header.

[Paul Gofman]

From: Paul Gofman <pgofman(a)codeweavers.com>

---
 dlls/winhttp/tests/winhttp.c | 39 ++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/dlls/winhttp/tests/winhttp.c b/dlls/winhttp/tests/winhttp.c
index 1fb9dad115f..5ca19aa8c8b 100644
--- a/dlls/winhttp/tests/winhttp.c
+++ b/dlls/winhttp/tests/winhttp.c
@@ -609,6 +609,7 @@ static void test_WinHttpAddHeaders(void)
         L"field: value ",
         L"name: value",
         L"name:",
+        L"g : value",
     };
     static const WCHAR test_indices[][6] =
     {
@@ -950,6 +951,9 @@ static void test_WinHttpAddHeaders(void)
     ret = WinHttpAddRequestHeaders(request, test_headers[13], ~0u, WINHTTP_ADDREQ_FLAG_ADD);
     ok(ret, "WinHttpAddRequestHeaders failed\n");
 
+    ret = WinHttpAddRequestHeaders(request, test_headers[16], ~0u, WINHTTP_ADDREQ_FLAG_ADD);
+    ok(!ret, "adding %s succeeded.\n", debugstr_w(test_headers[16]));
+
     index = 0;
     buffer[0] = 0;
     len = sizeof(buffer);
@@ -2287,6 +2291,13 @@ static const char redirectmsg[] =
 "Location: /temporary\r\n"
 "Connection: close\r\n\r\n";
 
+static const char badreplyheadermsg[] =
+"HTTP/1.1 200 OK\r\n"
+"Server: winetest\r\n"
+"SpaceAfterHdr  :   bad\r\n"
+"OkHdr: ok\r\n"
+"\r\n";
+
 static const char proxy_pac[] =
 "function FindProxyForURL(url, host) {\r\n"
 "    url = url.replace(/[:/]/g, '_');\r\n"
@@ -2558,7 +2569,7 @@ static DWORD CALLBACK server_thread(LPVOID param)
             ok(!!strstr(buffer, "Test5: Value5\r\n"), "Header missing from request %s.\n", debugstr_a(buffer));
             ok(!!strstr(buffer, "Test6: Value6\r\n"), "Header missing from request %s.\n", debugstr_a(buffer));
             ok(!!strstr(buffer, "Cookie: 111\r\n"), "Header missing from request %s.\n", debugstr_a(buffer));
-            send(c, okmsg, sizeof(okmsg) - 1, 0);
+            send(c, badreplyheadermsg, sizeof(badreplyheadermsg) - 1, 0);
         }
         if (strstr(buffer, "GET /proxy.pac"))
         {
@@ -3852,8 +3863,17 @@ static void test_not_modified(int port)
 
 static void test_bad_header( int port )
 {
-    WCHAR buffer[32];
+    static const WCHAR expected_headers[] =
+    {
+        L"HTTP/1.1 200 OK\r\n"
+        L"Server: winetest\r\n"
+        L"SpaceAfterHdr: bad\r\n"
+        L"OkHdr: ok\r\n"
+        L"\r\n"
+    };
+
     HINTERNET ses, con, req;
+    WCHAR buffer[512];
     DWORD index, len;
     unsigned int i;
     BOOL ret;
@@ -3912,6 +3932,20 @@ static void test_bad_header( int port )
     ret = WinHttpReceiveResponse( req, NULL );
     ok( ret, "failed to receive response %lu\n", GetLastError() );
 
+    len = sizeof(buffer);
+    ret = WinHttpQueryHeaders( req, WINHTTP_QUERY_CUSTOM, L"OkHdr", buffer, &len, WINHTTP_NO_HEADER_INDEX );
+    todo_wine ok( ret, "got error %lu.\n", GetLastError() );
+
+    len = sizeof(buffer);
+    ret = WinHttpQueryHeaders( req, WINHTTP_QUERY_RAW_HEADERS_CRLF, WINHTTP_HEADER_NAME_BY_INDEX, buffer, &len, WINHTTP_NO_HEADER_INDEX );
+    ok( ret, "got error %lu.\n", GetLastError() );
+    todo_wine ok( !wcscmp( buffer, expected_headers ), "got %s.\n", debugstr_w(buffer) );
+
+    len = sizeof(buffer);
+    ret = WinHttpQueryHeaders( req, WINHTTP_QUERY_CUSTOM, L"SpaceAfterHdr", buffer, &len, WINHTTP_NO_HEADER_INDEX );
+    todo_wine ok( ret, "got error %lu.\n", GetLastError() );
+    todo_wine ok( !wcscmp( buffer, L"bad" ), "got %s.\n", debugstr_w(buffer) );
+
     WinHttpCloseHandle( req );
     WinHttpCloseHandle( con );
     WinHttpCloseHandle( ses );
@@ -6035,6 +6069,7 @@ START_TEST (winhttp)
         CloseHandle(thread);
         return;
     }
+
     test_IWinHttpRequest(si.port);
     test_connection_info(si.port);
     test_basic_request(si.port, NULL, L"/basic");
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/5975

[PATCH 2/3] winhttp: Construct raw header from the parse result in read_reply().

[Paul Gofman]

From: Paul Gofman <pgofman(a)codeweavers.com>

---
 dlls/winhttp/request.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c
index 207f1ec5c61..2b8fa168947 100644
--- a/dlls/winhttp/request.c
+++ b/dlls/winhttp/request.c
@@ -2697,9 +2697,29 @@ static DWORD read_reply( struct request *request )
             free_header( header );
             break;
         }
+
+        lenW = wcslen( header->field );
+        assert( len - offset >= lenW + 1 );
+        memcpy( raw_headers + offset, header->field, lenW * sizeof(WCHAR) );
+        offset += lenW;
+
+        lenW = 2;
+        assert( len - offset >= lenW + 1 );
+        memcpy( raw_headers + offset, L": ", lenW * sizeof(WCHAR) );
+        offset += lenW;
+
+        lenW = wcslen( header->value );
+        assert( len - offset >= lenW + 1 );
+        memcpy( raw_headers + offset, header->value, lenW * sizeof(WCHAR) );
+        offset += lenW;
+
+        lenW = crlf_len;
+        assert( len - offset >= lenW + 1 );
+        memcpy( raw_headers + offset, L"\r\n", lenW * sizeof(WCHAR) );
+        offset += lenW;
+
+        raw_headers[offset] = 0;
         free_header( header );
-        memcpy( raw_headers + offset + buflen - 1, L"\r\n", sizeof(L"\r\n") );
-        offset += buflen + crlf_len - 1;
     }
 
     TRACE("raw headers: %s\n", debugstr_w(raw_headers));
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/5975

[PATCH 3/3] winhttp: Skip trailing spaces in reply header names.

[Paul Gofman]

From: Paul Gofman <pgofman(a)codeweavers.com>

---
 dlls/winhttp/request.c       | 23 ++++++++++++++++-------
 dlls/winhttp/tests/winhttp.c |  8 ++++----
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c
index 2b8fa168947..b86ff0beb1e 100644
--- a/dlls/winhttp/request.c
+++ b/dlls/winhttp/request.c
@@ -303,9 +303,9 @@ static BOOL valid_token_char( WCHAR c )
     }
 }
 
-static struct header *parse_header( const WCHAR *string, size_t string_len )
+static struct header *parse_header( const WCHAR *string, size_t string_len, BOOL reply )
 {
-    const WCHAR *p, *q;
+    const WCHAR *p, *q, *name_end;
     struct header *header;
     int len;
 
@@ -315,12 +315,21 @@ static struct header *parse_header( const WCHAR *string, size_t string_len )
         WARN("no ':' in line %s\n", debugstr_w(string));
         return NULL;
     }
-    if (q == string)
+    name_end = q;
+    if (reply)
+    {
+        while (name_end != string)
+        {
+            if (name_end[-1] != ' ') break;
+            --name_end;
+        }
+    }
+    if (name_end == string)
     {
         WARN("empty field name in line %s\n", debugstr_w(string));
         return NULL;
     }
-    while (*p != ':')
+    while (p != name_end)
     {
         if (!valid_token_char( *p ))
         {
@@ -329,7 +338,7 @@ static struct header *parse_header( const WCHAR *string, size_t string_len )
         }
         p++;
     }
-    len = q - string;
+    len = name_end - string;
     if (!(header = calloc( 1, sizeof(*header) ))) return NULL;
     if (!(header->field = malloc( (len + 1) * sizeof(WCHAR) )))
     {
@@ -491,7 +500,7 @@ DWORD add_request_headers( struct request *request, const WCHAR *headers, DWORD
         while (*q == '\r' || *q == '\n')
             ++q;
 
-        if ((header = parse_header( p, end - p )))
+        if ((header = parse_header( p, end - p, FALSE )))
         {
             ret = process_header( request, header->field, header->value, flags, TRUE );
             free_header( header );
@@ -2691,7 +2700,7 @@ static DWORD read_reply( struct request *request )
         }
         lenW = MultiByteToWideChar( CP_ACP, 0, buffer, buflen, raw_headers + offset, buflen );
 
-        if (!(header = parse_header( raw_headers + offset, lenW - 1 ))) break;
+        if (!(header = parse_header( raw_headers + offset, lenW - 1, TRUE ))) break;
         if ((ret = process_header( request, header->field, header->value, WINHTTP_ADDREQ_FLAG_ADD, FALSE )))
         {
             free_header( header );
diff --git a/dlls/winhttp/tests/winhttp.c b/dlls/winhttp/tests/winhttp.c
index 5ca19aa8c8b..2252551f32c 100644
--- a/dlls/winhttp/tests/winhttp.c
+++ b/dlls/winhttp/tests/winhttp.c
@@ -3934,17 +3934,17 @@ static void test_bad_header( int port )
 
     len = sizeof(buffer);
     ret = WinHttpQueryHeaders( req, WINHTTP_QUERY_CUSTOM, L"OkHdr", buffer, &len, WINHTTP_NO_HEADER_INDEX );
-    todo_wine ok( ret, "got error %lu.\n", GetLastError() );
+    ok( ret, "got error %lu.\n", GetLastError() );
 
     len = sizeof(buffer);
     ret = WinHttpQueryHeaders( req, WINHTTP_QUERY_RAW_HEADERS_CRLF, WINHTTP_HEADER_NAME_BY_INDEX, buffer, &len, WINHTTP_NO_HEADER_INDEX );
     ok( ret, "got error %lu.\n", GetLastError() );
-    todo_wine ok( !wcscmp( buffer, expected_headers ), "got %s.\n", debugstr_w(buffer) );
+    ok( !wcscmp( buffer, expected_headers ), "got %s.\n", debugstr_w(buffer) );
 
     len = sizeof(buffer);
     ret = WinHttpQueryHeaders( req, WINHTTP_QUERY_CUSTOM, L"SpaceAfterHdr", buffer, &len, WINHTTP_NO_HEADER_INDEX );
-    todo_wine ok( ret, "got error %lu.\n", GetLastError() );
-    todo_wine ok( !wcscmp( buffer, L"bad" ), "got %s.\n", debugstr_w(buffer) );
+    ok( ret, "got error %lu.\n", GetLastError() );
+    ok( !wcscmp( buffer, L"bad" ), "got %s.\n", debugstr_w(buffer) );
 
     WinHttpCloseHandle( req );
     WinHttpCloseHandle( con );
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/5975

[PATCH 3/3] winhttp: Skip trailing spaces in reply header names.

[Marvin]

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=146759

Your paranoid android.


=== debian11b (64 bit WoW report) ===

kernel32:
comm.c:1574: Test failed: AbortWaitCts hComPortEvent failed
comm.c:1586: Test failed: Unexpected time 1001, expected around 500

[PATCH 0/3] MR5975: winhttp: Skip trailing spaces in reply headers. - approved

[Hans Leidekker (＠hans)]

This merge request was approved by Hans Leidekker.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5975