[PATCH v4 0/1] MR11347: wininet: Fix out-of-bounds read in InternetCrackUrlW for empty file paths.
InternetCrackUrlW would read past the end of the string when parsing a \"file:\" URL with an empty path and an immediate fragment, e.g., \"file://#\". Add a check for this case and return ERROR_INVALID_PARAMETER as expected. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=59973 -- v4: wininet: Fix out-of-bounds read in InternetCrackUrlW for empty file paths. https://gitlab.winehq.org/wine/wine/-/merge_requests/11347
From: Kyle Yang <kyle.chaoxin.yang@gmail.com> InternetCrackUrlW would read past the end of the string when parsing a \"file:\" URL with an empty path and an immediate fragment, e.g., \"file://#\". Add a check for this case and return ERROR_INVALID_PARAMETER as expected. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=59973 --- dlls/wininet/internet.c | 17 +++++++++++--- dlls/wininet/tests/url.c | 51 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/dlls/wininet/internet.c b/dlls/wininet/internet.c index 7eb9dd710cb..08fa47c832b 100644 --- a/dlls/wininet/internet.c +++ b/dlls/wininet/internet.c @@ -2300,12 +2300,23 @@ BOOL WINAPI InternetCrackUrlW(const WCHAR *lpszUrl, DWORD dwUrlLength, DWORD dwF WCHAR tmppath[MAX_PATH]; if (*lpszcp == '/') { + HRESULT hr; len = MAX_PATH; - PathCreateFromUrlW(lpszUrl, tmppath, &len, 0); + hr = PathCreateFromUrlW(lpszUrl, tmppath, &len, 0); + if (FAILED(hr)) + { + SetLastError(ERROR_INSUFFICIENT_BUFFER); + return FALSE; + } } else { WCHAR *iter; + if (len >= MAX_PATH) + { + SetLastError(ERROR_INSUFFICIENT_BUFFER); + return FALSE; + } memcpy(tmppath, lpszcp, len * sizeof(WCHAR)); tmppath[len] = '\0'; @@ -2317,9 +2328,9 @@ BOOL WINAPI InternetCrackUrlW(const WCHAR *lpszUrl, DWORD dwUrlLength, DWORD dwF } } /* if ends in \. or \.. append a backslash */ - if (tmppath[len - 1] == '.' && + if (len >= 2 && tmppath[len - 1] == '.' && (tmppath[len - 2] == '\\' || - (tmppath[len - 2] == '.' && tmppath[len - 3] == '\\'))) + (len >= 3 && tmppath[len - 2] == '.' && tmppath[len - 3] == '\\'))) { if (len < MAX_PATH - 1) { diff --git a/dlls/wininet/tests/url.c b/dlls/wininet/tests/url.c index 2177d651f08..a6e9001f539 100644 --- a/dlls/wininet/tests/url.c +++ b/dlls/wininet/tests/url.c @@ -671,6 +671,23 @@ static void InternetCrackUrl_test(void) ok(ret, "InternetCrackUrlA failed with error %ld\n", GetLastError()); ok(urlComponents.dwUrlPathLength == 0, "Expected dwUrlPathLength of 0, got %ld\n", urlComponents.dwUrlPathLength); + + /* file:// URLs with empty path and fragment/query immediately after // + * These previously caused a crash (STATUS_STACK_OVERFLOW) due to tmppath[-1] + * array underflow in InternetCrackUrlW when path length was 0. */ + copy_compsA(&urlSrc, &urlComponents, 32, 1024, 1024, 1024, 1024, 1024); + SetLastError(0xdeadbeef); + ret = InternetCrackUrlA("file://#:] ", 10, ICU_DECODE, &urlComponents); + ok(ret, "InternetCrackUrlA failed with error %ld\n", GetLastError()); + ok(urlComponents.nScheme == INTERNET_SCHEME_FILE, + "Expected INTERNET_SCHEME_FILE, got %d\n", urlComponents.nScheme); + + copy_compsA(&urlSrc, &urlComponents, 32, 1024, 1024, 1024, 1024, 1024); + SetLastError(0xdeadbeef); + ret = InternetCrackUrlA("file://#fragment", 0, 0, &urlComponents); + ok(ret, "InternetCrackUrlA failed with error %ld\n", GetLastError()); + ok(urlComponents.nScheme == INTERNET_SCHEME_FILE, + "Expected INTERNET_SCHEME_FILE, got %d\n", urlComponents.nScheme); } static void InternetCrackUrlW_test(void) @@ -878,6 +895,40 @@ static void InternetCrackUrlW_test(void) ok(r, "InternetCrackUrlW failed with error %ld\n", GetLastError()); ok(comp.dwUrlPathLength == 0, "Expected dwUrlPathLength of 0, got %ld\n", comp.dwUrlPathLength); + + /* file:// URLs with empty path and fragment/query immediately after // + * These previously caused a crash (STATUS_STACK_OVERFLOW) due to tmppath[-1] + * array underflow in InternetCrackUrlW when path length was 0. */ + { + static const WCHAR file_hash[] = L"file://#:]"; + static const WCHAR file_frag[] = L"file://#frag"; + + host[0] = 0; + urlpart[0] = 0; + memset(&comp, 0, sizeof(comp)); + comp.dwStructSize = sizeof(comp); + comp.lpszHostName = host; + comp.dwHostNameLength = ARRAY_SIZE(host); + comp.lpszUrlPath = urlpart; + comp.dwUrlPathLength = ARRAY_SIZE(urlpart); + r = InternetCrackUrlW(file_hash, 10, ICU_DECODE, &comp); + ok(r, "InternetCrackUrlW(file_hash) failed with error %ld\n", GetLastError()); + ok(comp.nScheme == INTERNET_SCHEME_FILE, + "Expected INTERNET_SCHEME_FILE, got %d\n", comp.nScheme); + + host[0] = 0; + urlpart[0] = 0; + memset(&comp, 0, sizeof(comp)); + comp.dwStructSize = sizeof(comp); + comp.lpszHostName = host; + comp.dwHostNameLength = ARRAY_SIZE(host); + comp.lpszUrlPath = urlpart; + comp.dwUrlPathLength = ARRAY_SIZE(urlpart); + r = InternetCrackUrlW(file_frag, 0, 0, &comp); + ok(r, "InternetCrackUrlW(file_frag) failed with error %ld\n", GetLastError()); + ok(comp.nScheme == INTERNET_SCHEME_FILE, + "Expected INTERNET_SCHEME_FILE, got %d\n", comp.nScheme); + } } static void fill_url_components(URL_COMPONENTSA *lpUrlComponents) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11347
Jacek Caban (@jacek) commented about dlls/wininet/internet.c:
if (*lpszcp == '/') { + HRESULT hr; len = MAX_PATH; - PathCreateFromUrlW(lpszUrl, tmppath, &len, 0); + hr = PathCreateFromUrlW(lpszUrl, tmppath, &len, 0); + if (FAILED(hr)) + { + SetLastError(ERROR_INSUFFICIENT_BUFFER); + return FALSE; + } } else { WCHAR *iter; + if (len >= MAX_PATH)
This seems unrelated to the problem from the commit message. Is it a limitation on Windows too? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11347#note_145861
Jacek Caban (@jacek) commented about dlls/wininet/tests/url.c:
ok(r, "InternetCrackUrlW failed with error %ld\n", GetLastError()); ok(comp.dwUrlPathLength == 0, "Expected dwUrlPathLength of 0, got %ld\n", comp.dwUrlPathLength); + + /* file:// URLs with empty path and fragment/query immediately after // + * These previously caused a crash (STATUS_STACK_OVERFLOW) due to tmppath[-1] + * array underflow in InternetCrackUrlW when path length was 0. */ + { + static const WCHAR file_hash[] = L"file://#:]"; + static const WCHAR file_frag[] = L"file://#frag";
Please avoid blocks just for sake of declaring variables. In this case, you don't really need those variables and you could just inline those strings. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11347#note_145862
On Fri Jul 17 06:25:20 2026 +0000, Jacek Caban wrote:
This seems unrelated to the problem from the commit message. Is it a limitation on Windows too? You're right, I should focus on this fix.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11347#note_145903
On Fri Jul 17 06:30:37 2026 +0000, Jacek Caban wrote:
Please avoid blocks just for sake of declaring variables. In this case, you don't really need those variables and you could just inline those strings. You're right, thank you. I will inline the URL strings directly into the InternetCrackUrlW calls and remove the variable declarations.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11347#note_145904
participants (3)
-
Jacek Caban (＠jacek) -
Kyle Yang
-
Kyle Yang (＠kyle)