[PATCH v3 1/3] wininet: Return error from HttpQueryInfo if number argument is invalid.
Signed-off-by: Daniel Lehman <dlehman25(a)gmail.com> --- v3: - more tests - check errno v2: - just use strtoulW with overflow check - make this patch first since it's really 0003 that needs it --- dlls/wininet/http.c | 22 +++++++++-- dlls/wininet/tests/http.c | 82 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 3 deletions(-) diff --git a/dlls/wininet/http.c b/dlls/wininet/http.c index 538fc2bf6b..3230749d40 100644 --- a/dlls/wininet/http.c +++ b/dlls/wininet/http.c @@ -42,6 +42,7 @@ #include <stdio.h> #include <time.h> #include <assert.h> +#include <errno.h> #include "windef.h" #include "winbase.h" @@ -3724,9 +3725,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 || (value == ULONG_MAX && errno == ERANGE)) + { + 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 cb96abe612..fcbd95b837 100644 --- a/dlls/wininet/tests/http.c +++ b/dlls/wininet/tests/http.c @@ -23,6 +23,7 @@ #include <stdarg.h> #include <stdio.h> #include <stdlib.h> +#include <limits.h> #include "windef.h" #include "winbase.h" @@ -2061,6 +2062,12 @@ static const char okmsg2[] = "Set-Cookie: two\r\n" "\r\n"; +static DWORD64 content_length; +static const char largemsg[] = +"HTTP/1.1 200 OK\r\n" +"Content-Length: %I64u\r\n" +"\r\n"; + static const char notokmsg[] = "HTTP/1.1 400 Bad Request\r\n" "Server: winetest\r\n" @@ -2455,6 +2462,12 @@ static DWORD CALLBACK server_thread(LPVOID param) { send(c, okmsg, sizeof(okmsg)-1, 0); } + if (strstr(buffer, "HEAD /test_large_content")) + { + char msg[sizeof(largemsg) + 16]; + sprintf(msg, largemsg, content_length); + send(c, msg, strlen(msg), 0); + } shutdown(c, 2); closesocket(c); c = -1; @@ -5515,6 +5528,74 @@ static void test_remove_dot_segments(int port) close_request(&req); } +struct large_test +{ + DWORD64 content_length; + BOOL ret; +}; + +static void test_large_content(int port) +{ + struct large_test tests[] = { + { 0, TRUE }, + { UINT_MAX-1, TRUE }, + { UINT_MAX, TRUE }, + { (DWORD64)UINT_MAX+1, FALSE }, + { ~0, FALSE }, + }; + test_request_t req; + DWORD sizelen, len; + DWORD64 len64; + BOOL ret; + size_t i; + + open_simple_request(&req, "localhost", port, "HEAD", "/test_large_content"); + + for (i = 0; i < ARRAY_SIZE(tests); i++) + { + content_length = tests[i].content_length; + ret = HttpSendRequestA(req.request, NULL, 0, NULL, 0); + ok(ret, "HttpSendRequest failed: %u\n", GetLastError()); + + len = ~0; + sizelen = sizeof(len); + SetLastError(0xdeadbeef); + ret = HttpQueryInfoA(req.request, HTTP_QUERY_FLAG_NUMBER|HTTP_QUERY_CONTENT_LENGTH, + &len, &sizelen, 0); + if (tests[i].ret) + { + ok(ret, "HttpQueryInfo should have succeeded\n"); + ok(GetLastError() == ERROR_SUCCESS || + broken(GetLastError() == 0xdeadbeef), /* xp, 2k8, vista */ + "expected ERROR_SUCCESS, got %x\n", GetLastError()); + ok(len == (DWORD)tests[i].content_length, "expected %u, got %u\n", + (DWORD)tests[i].content_length, len); + } + else + { + ok(!ret, "HttpQueryInfo should have failed\n"); + ok(GetLastError() == ERROR_HTTP_INVALID_HEADER, + "expected ERROR_HTTP_INVALID_HEADER, got %x\n", GetLastError()); + ok(len == ~0, "expected ~0, got %u\n", len); + } + ok(sizelen == sizeof(DWORD), "sizelen %u\n", sizelen); + } + + /* test argument size */ + len64 = ~0; + sizelen = sizeof(len64); + 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, + "expected ERROR_HTTP_INVALID_HEADER, got %x\n", GetLastError()); + ok(sizelen == sizeof(DWORD64), "sizelen %u\n", sizelen); + ok(len64 == ~0, "len64 %x%08x\n", (DWORD)(len64 >> 32), (DWORD)len64); + + close_request(&req); +} + static void test_http_connection(void) { struct server_info si; @@ -5573,6 +5654,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=56204 Your paranoid android. === 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 === debian10 (64 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:
@@ -44,6 +44,7 @@ WINE_1.0 strstrW; strtolW; strtoulW; + strtoullW; struprW; tolowerW; toupperW;
We don't want to modify the libwine interface. You should use a Win32 API function like _wtoi64. -- Alexandre Julliard julliard(a)winehq.org
Signed-off-by: Daniel Lehman <dlehman25(a)gmail.com> --- v3 - add test --- dlls/wininet/http.c | 24 ++++++++++++++---------- dlls/wininet/internet.h | 6 +++--- dlls/wininet/tests/http.c | 20 ++++++++++++++++++++ 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/dlls/wininet/http.c b/dlls/wininet/http.c index 3230749d40..8564cc12f4 100644 --- a/dlls/wininet/http.c +++ b/dlls/wininet/http.c @@ -2441,7 +2441,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; @@ -2645,7 +2645,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) { @@ -2751,7 +2751,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; } @@ -2873,6 +2873,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; @@ -2881,10 +2882,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; @@ -2910,7 +2914,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) { @@ -3300,7 +3304,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; @@ -4923,7 +4927,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 */ diff --git a/dlls/wininet/tests/http.c b/dlls/wininet/tests/http.c index fcbd95b837..a4521a94a4 100644 --- a/dlls/wininet/tests/http.c +++ b/dlls/wininet/tests/http.c @@ -5545,7 +5545,9 @@ static void test_large_content(int port) }; test_request_t req; DWORD sizelen, len; + DWORD read_size; DWORD64 len64; + char buf[16]; BOOL ret; size_t i; @@ -5594,6 +5596,24 @@ static void test_large_content(int port) ok(len64 == ~0, "len64 %x%08x\n", (DWORD)(len64 >> 32), (DWORD)len64); close_request(&req); + + /* test internal use of HttpQueryInfo on large size */ + open_read_test_request(port, &req, + "HTTP/1.1 200 OK\r\n" + "Server: winetest\r\n" + "Content-Length: 4294967296\r\n" + "\r\n" + "xx"); + read_expect_async(req.request, buf, 4, &read_size, "xx"); + send_response_and_wait("yy1234567890", FALSE, buf, &read_size, "xxyy", 4, 0, 2); + read_expect_sync_data(req.request, buf, 10, "1234567890"); + + SET_EXPECT(INTERNET_STATUS_CLOSING_CONNECTION); + SET_EXPECT(INTERNET_STATUS_CONNECTION_CLOSED); + close_async_handle(req.session, 2); + CHECK_NOTIFIED(INTERNET_STATUS_CLOSING_CONNECTION); + CHECK_NOTIFIED(INTERNET_STATUS_CONNECTION_CLOSED); + close_connection(); } static void test_http_connection(void) -- 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=56205 Your paranoid android. === wvistau64_he (32 bit report) === wininet: http.c:5910: Test failed: req_error = 12152 http.c:5926: Test failed: flags = 8, expected 0 http.c:5931: Test failed: InternetReadFile failed: 12019 http.c:5932: Test failed: size = 0 === debian10 (32 bit French report) === wininet: http.c:4573: 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=56203 Your paranoid android. === debian10 (32 bit Chinese:China report) === wininet: http.c:4573: Test failed: expected 1 pending read, got 2
participants (3)
-
Alexandre Julliard -
Daniel Lehman -
Marvin