[PATCH v2 1/3] wininet: Return error from HttpQueryInfo if number argument is invalid.
Signed-off-by: Daniel Lehman <dlehman25(a)gmail.com> --- v2: - just use strtoulW with overflow check - make this patch first since it's really 0003 that needs it --- dlls/wininet/http.c | 21 +++++++++++++--- dlls/wininet/tests/http.c | 51 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 3 deletions(-) diff --git a/dlls/wininet/http.c b/dlls/wininet/http.c index 538fc2bf6b..f771356760 100644 --- a/dlls/wininet/http.c +++ b/dlls/wininet/http.c @@ -3724,9 +3724,24 @@ static DWORD HTTP_HttpQueryInfoW(http_request_t *request, DWORD dwInfoLevel, /* coalesce value to requested type */ if (dwInfoLevel & HTTP_QUERY_FLAG_NUMBER && lpBuffer) { - *(int *)lpBuffer = atoiW(lphttpHdr->lpszValue); - TRACE(" returning number: %d\n", *(int *)lpBuffer); - } + unsigned long value; + + if (*lpdwBufferLength != sizeof(DWORD)) + { + LeaveCriticalSection( &request->headers_section ); + return ERROR_HTTP_INVALID_HEADER; + } + + value = strtoulW( lphttpHdr->lpszValue, NULL, 10 ); + if (value >= UINT_MAX) + { + LeaveCriticalSection( &request->headers_section ); + return ERROR_HTTP_INVALID_HEADER; + } + + *(DWORD *)lpBuffer = value; + TRACE(" returning number: %u\n", *(DWORD *)lpBuffer); + } else if (dwInfoLevel & HTTP_QUERY_FLAG_SYSTEMTIME && lpBuffer) { time_t tmpTime; diff --git a/dlls/wininet/tests/http.c b/dlls/wininet/tests/http.c index 6db33e4033..076c6ce5a4 100644 --- a/dlls/wininet/tests/http.c +++ b/dlls/wininet/tests/http.c @@ -2061,6 +2061,11 @@ static const char okmsg2[] = "Set-Cookie: two\r\n" "\r\n"; +static const char notok4gb[] = +"HTTP/1.1 200 OK\r\n" +"Content-Length: 4294967296\r\n" +"\r\n"; + static const char notokmsg[] = "HTTP/1.1 400 Bad Request\r\n" "Server: winetest\r\n" @@ -2455,6 +2460,10 @@ static DWORD CALLBACK server_thread(LPVOID param) { send(c, okmsg, sizeof(okmsg)-1, 0); } + if (strstr(buffer, "HEAD /test_4GB")) + { + send(c, notok4gb, sizeof(notok4gb)-1, 0); + } shutdown(c, 2); closesocket(c); c = -1; @@ -5515,6 +5524,47 @@ static void test_remove_dot_segments(int port) close_request(&req); } +static void test_large_content(int port) +{ + test_request_t req; + DWORD len, len2; + DWORD64 len64; + BOOL ret; + + open_simple_request(&req, "localhost", port, "HEAD", "/test_4GB"); + + ret = HttpSendRequestA(req.request, NULL, 0, NULL, 0); + ok(ret, "HttpSendRequest failed: %u\n", GetLastError()); + + /* test argument size */ + len = sizeof(len64); + len64 = ~0; + SetLastError(0xdeadbeef); + ret = HttpQueryInfoA(req.request, HTTP_QUERY_FLAG_NUMBER|HTTP_QUERY_CONTENT_LENGTH, + &len64, &len, 0); + ok(!ret, "HttpQueryInfo should have failed\n"); + ok(GetLastError() == ERROR_HTTP_INVALID_HEADER, + "HttpQueryInfo should have set last error to ERROR_HTTP_INVALID_HEADER instead of %u\n", + GetLastError()); + ok(len == sizeof(DWORD64), "len = %u\n", len); + ok(len64 == ~0, "len64 = %x%08x\n", (DWORD)(len64 >> 32), (DWORD)len64); + + /* test overflow */ + len = sizeof(len2); + len2 = ~0; + SetLastError(0xdeadbeef); + ret = HttpQueryInfoA(req.request, HTTP_QUERY_FLAG_NUMBER|HTTP_QUERY_CONTENT_LENGTH, + &len2, &len, 0); + ok(!ret, "HttpQueryInfo should have failed\n"); + ok(GetLastError() == ERROR_HTTP_INVALID_HEADER, + "HttpQueryInfo should have set last error to ERROR_HTTP_INVALID_HEADER instead of %u\n", + GetLastError()); + ok(len == sizeof(DWORD), "len = %u\n", len); + ok(len2 == ~0, "len2 = %x\n", len2); + + close_request(&req); +} + static void test_http_connection(void) { struct server_info si; @@ -5573,6 +5623,7 @@ static void test_http_connection(void) test_redirect(si.port); test_persistent_connection(si.port); test_remove_dot_segments(si.port); + test_large_content(si.port); /* send the basic request again to shutdown the server thread */ test_basic_request(si.port, "GET", "/quit"); -- 2.17.1
Signed-off-by: Daniel Lehman <dlehman25(a)gmail.com> --- include/wine/unicode.h | 1 + libs/port/string.c | 113 +++++++++++++++++++++++++++++++++++++++++ libs/wine/wine.map | 1 + 3 files changed, 115 insertions(+) diff --git a/include/wine/unicode.h b/include/wine/unicode.h index ff6f6568a5..5ee53e74a5 100644 --- a/include/wine/unicode.h +++ b/include/wine/unicode.h @@ -108,6 +108,7 @@ extern int memicmpW( const WCHAR *str1, const WCHAR *str2, int n ); extern WCHAR *strstrW( const WCHAR *str, const WCHAR *sub ); extern long int strtolW( const WCHAR *nptr, WCHAR **endptr, int base ); extern unsigned long int strtoulW( const WCHAR *nptr, WCHAR **endptr, int base ); +extern unsigned long long strtoullW( const WCHAR *nptr, WCHAR **endptr, int base ); extern int sprintfW( WCHAR *str, const WCHAR *format, ... ); extern int snprintfW( WCHAR *str, size_t len, const WCHAR *format, ... ); extern int vsprintfW( WCHAR *str, const WCHAR *format, va_list valist ); diff --git a/libs/port/string.c b/libs/port/string.c index c3388bcc7d..0ff4700664 100644 --- a/libs/port/string.c +++ b/libs/port/string.c @@ -301,6 +301,119 @@ noconv: } +unsigned long long strtoullW( const WCHAR *nptr, WCHAR **endptr, int base ) +{ + int negative; + register unsigned long long cutoff; + register unsigned int cutlim; + register unsigned long long i; + register const WCHAR *s; + register WCHAR c; + const WCHAR *save, *end; + int overflow; + + if (base < 0 || base == 1 || base > 36) return 0; + + save = s = nptr; + + /* Skip white space. */ + while (isspaceW (*s)) + ++s; + if (!*s) goto noconv; + + /* Check for a sign. */ + negative = 0; + if (*s == '-') + { + negative = 1; + ++s; + } + else if (*s == '+') + ++s; + + /* Recognize number prefix and if BASE is zero, figure it out ourselves. */ + if (*s == '0') + { + if ((base == 0 || base == 16) && toupperW(s[1]) == 'X') + { + s += 2; + base = 16; + } + else if (base == 0) + base = 8; + } + else if (base == 0) + base = 10; + + /* Save the pointer so we can check later if anything happened. */ + save = s; + end = NULL; + + cutoff = ULLONG_MAX / (unsigned long long) base; + cutlim = ULLONG_MAX % (unsigned long long) base; + + overflow = 0; + i = 0; + c = *s; + for (;c != '\0'; c = *++s) + { + if (s == end) + break; + if (c >= '0' && c <= '9') + c -= '0'; + else if (isalphaW (c)) + c = toupperW (c) - 'A' + 10; + else + break; + if ((int) c >= base) + break; + /* Check for overflow. */ + if (i > cutoff || (i == cutoff && c > cutlim)) + overflow = 1; + else + { + i *= (unsigned long long) base; + i += c; + } + } + + /* Check if anything actually happened. */ + if (s == save) + goto noconv; + + /* Store in ENDPTR the address of one character + past the last character we converted. */ + if (endptr != NULL) + *endptr = (WCHAR *)s; + + if (overflow) + { + errno = ERANGE; + return ULLONG_MAX; + } + + /* Return the result of the appropriate sign. */ + return negative ? -i : i; + +noconv: + /* We must handle a special case here: the base is 0 or 16 and the + first two characters are '0' and 'x', but the rest are not + hexadecimal digits. This is no error case. We return 0 and + ENDPTR points to the `x`. */ + if (endptr != NULL) + { + if (save - nptr >= 2 && toupperW (save[-1]) == 'X' + && save[-2] == '0') + *endptr = (WCHAR *)&save[-1]; + else + /* There was no number to convert. */ + *endptr = (WCHAR *)nptr; + } + + return 0; +} + + /* format a WCHAR string according to a printf format; helper for vsnprintfW */ static size_t format_string( WCHAR *buffer, size_t len, const char *format, const WCHAR *str, int str_len ) { diff --git a/libs/wine/wine.map b/libs/wine/wine.map index 2159fac852..741ae759de 100644 --- a/libs/wine/wine.map +++ b/libs/wine/wine.map @@ -44,6 +44,7 @@ WINE_1.0 strstrW; strtolW; strtoulW; + strtoullW; struprW; tolowerW; toupperW; -- 2.17.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=55902 Your paranoid android. === debian10 (32 bit report) === wininet: http.c:4569: Test failed: expected 1 pending read, got 2
Signed-off-by: Daniel Lehman <dlehman25(a)gmail.com> --- dlls/wininet/http.c | 24 ++++++++++++++---------- dlls/wininet/internet.h | 6 +++--- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/dlls/wininet/http.c b/dlls/wininet/http.c index f771356760..da1d21fe39 100644 --- a/dlls/wininet/http.c +++ b/dlls/wininet/http.c @@ -2440,7 +2440,7 @@ static void create_cache_entry(http_request_t *req) return; } - b = CreateUrlCacheEntryW(url, req->contentLength == ~0u ? 0 : req->contentLength, NULL, file_name, 0); + b = CreateUrlCacheEntryW(url, req->contentLength == ~0 ? 0 : req->contentLength, NULL, file_name, 0); if(!b) { WARN("Could not create cache entry: %08x\n", GetLastError()); return; @@ -2644,7 +2644,7 @@ static DWORD netconn_drain_content(data_stream_t *stream, http_request_t *req, B int len, res; size_t size; - if(netconn_stream->content_length == ~0u) + if(netconn_stream->content_length == ~0) return WSAEISCONN; while(netconn_stream->content_read < netconn_stream->content_length) { @@ -2750,7 +2750,7 @@ static DWORD chunked_read(data_stream_t *stream, http_request_t *req, BYTE *buf, TRACE("reading %u byte chunk\n", chunked_stream->chunk_size); chunked_stream->buf_size++; chunked_stream->buf_pos--; - if(req->contentLength == ~0u) req->contentLength = chunked_stream->chunk_size; + if(req->contentLength == ~0) req->contentLength = chunked_stream->chunk_size; else req->contentLength += chunked_stream->chunk_size; chunked_stream->state = CHUNKED_STREAM_STATE_DISCARD_EOL_AFTER_SIZE; } @@ -2872,6 +2872,7 @@ static DWORD set_content_length(http_request_t *request) { static const WCHAR szChunked[] = {'c','h','u','n','k','e','d',0}; static const WCHAR headW[] = {'H','E','A','D',0}; + WCHAR contentLength[32]; WCHAR encoding[20]; DWORD size; @@ -2880,10 +2881,13 @@ static DWORD set_content_length(http_request_t *request) return ERROR_SUCCESS; } - size = sizeof(request->contentLength); - if (HTTP_HttpQueryInfoW(request, HTTP_QUERY_FLAG_NUMBER|HTTP_QUERY_CONTENT_LENGTH, - &request->contentLength, &size, NULL) != ERROR_SUCCESS) - request->contentLength = ~0u; + size = sizeof(contentLength); + if (HTTP_HttpQueryInfoW(request, HTTP_QUERY_CONTENT_LENGTH, + contentLength, &size, NULL) == ERROR_SUCCESS) + request->contentLength = strtoullW(contentLength, NULL, 10); + else + request->contentLength = ~0; + request->netconn_stream.content_length = request->contentLength; request->netconn_stream.content_read = request->read_size; @@ -2909,7 +2913,7 @@ static DWORD set_content_length(http_request_t *request) } request->data_stream = &chunked_stream->data_stream; - request->contentLength = ~0u; + request->contentLength = ~0; } if(request->hdr.decoding) { @@ -3299,7 +3303,7 @@ static DWORD HTTP_HttpOpenRequestW(http_session_t *session, request->hdr.dwFlags = dwFlags; request->hdr.dwContext = dwContext; request->hdr.decoding = session->hdr.decoding; - request->contentLength = ~0u; + request->contentLength = ~0; request->netconn_stream.data_stream.vtbl = &netconn_stream_vtbl; request->data_stream = &request->netconn_stream.data_stream; @@ -4922,7 +4926,7 @@ static DWORD HTTP_HttpSendRequestW(http_request_t *request, LPCWSTR lpszHeaders, loop_next = FALSE; if(redirected) { - request->contentLength = ~0u; + request->contentLength = ~0; request->bytesToWrite = 0; } diff --git a/dlls/wininet/internet.h b/dlls/wininet/internet.h index f712fb6b64..40e51bec32 100644 --- a/dlls/wininet/internet.h +++ b/dlls/wininet/internet.h @@ -335,8 +335,8 @@ typedef struct { typedef struct { data_stream_t data_stream; - DWORD content_length; - DWORD content_read; + ULONGLONG content_length; + ULONGLONG content_read; } netconn_stream_t; #define READ_BUFFER_SIZE 8192 @@ -372,7 +372,7 @@ typedef struct struct HttpAuthInfo *proxyAuthInfo; CRITICAL_SECTION read_section; /* section to protect the following fields */ - DWORD contentLength; /* total number of bytes to be read */ + ULONGLONG contentLength; /* total number of bytes to be read */ BOOL read_gzip; /* are we reading in gzip mode? */ DWORD read_pos; /* current read position in read_buf */ DWORD read_size; /* valid data size in read_buf */ -- 2.17.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=55903 Your paranoid android. === debian10 (32 bit report) === wininet: http.c:4569: Test failed: expected 1 pending read, got 2
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=55901 Your paranoid android. === wvistau64_fr (32 bit report) === wininet: http.c:5859: Test failed: req_error = 12152 http.c:5875: Test failed: flags = 8, expected 0 http.c:5880: Test failed: InternetReadFile failed: 12019 http.c:5881: Test failed: size = 0 === w1064v1809 (32 bit report) === wininet: http.c:4559: Test failed: expected ib.dwBufferLength != 0 http.c:4569: Test failed: expected 1 pending read, got 2 === wvistau64 (64 bit report) === wininet: http.c:4647: Test failed: expected bytes != 0 http.c:4657: Test failed: expected 1 pending read, got 2 === debian10 (32 bit WoW report) === wininet: ftp.c:81: Test failed: Expected ERROR_INTERNET_LOGIN_FAILURE, got 12110 ftp.c:112: Test failed: InternetConnect failed : 12110 ftp.c:113: Test failed: ERROR_SUCCESS, got 12110 ftp.c:122: Test failed: Expected ERROR_INTERNET_LOGIN_FAILURE, got 12110
Daniel Lehman <dlehman25(a)gmail.com> writes:
+ value = strtoulW( lphttpHdr->lpszValue, NULL, 10 ); + if (value >= UINT_MAX) + { + LeaveCriticalSection( &request->headers_section ); + return ERROR_HTTP_INVALID_HEADER; + }
Is UINT_MAX really supposed to fail? Otherwise you need an errno check. Either way, some more test cases would be a good idea. -- Alexandre Julliard julliard(a)winehq.org
Is UINT_MAX really supposed to fail? Otherwise you need an errno check. Either way, some more test cases would be a good idea.
For the given test for 4GB: on 32-bit, strtoulW returns 32-bit ULONG_MAX and sets errno to ERANGE on 64-bit, strtoulW returns 0x100000000 and does not set errno since the return is DWORD, I chose UINT_MAX since it's the same for both 32 and 64-bit and shortened the check to just "value >= UINT_MAX" will add more tests, though thanks daniel
participants (4)
-
Alexandre Julliard -
Daniel Lehman -
Daniel Lehman -
Marvin