[PATCH v3 0/4] MR11255: urlmon: Fix URL cache path handling for long extensions
This series fixes URL cache filename generation when URLs contain overly long file extensions. CreateUrlCacheEntryW has no output buffer size parameter, so the generated cache path must remain within MAX_PATH; overly long extensions are now ignored instead of being appended to a path that cannot safely fit. URLDownloadToCacheFileA/W now uses the caller-provided output buffer length correctly, includes the null terminator in bounds checks, and removes temporary cache files when download or cache commit fails. Regression tests were added for both WinINet cache entry creation and URLMon cache download paths to cover long extension handling, buffer boundary checks, and stale temporary file cleanup. -- v3: urlmon: Fix URLDownloadToCacheFile buffer handling. wininet: Ignore overly long cache file extensions. urlmon/tests: Cover URLDownloadToCacheFile buffer bounds. wininet/tests: Cover long CreateUrlCacheEntryW extensions. https://gitlab.winehq.org/wine/wine/-/merge_requests/11255
From: Jiajin Cui <cuijiajin@uniontech.com> Verify that CreateUrlCacheEntryW does not write paths longer than MAX_PATH when the file extension fills the output buffer. Signed-off-by: Jiajin Cui <cuijiajin@uniontech.com> --- dlls/wininet/tests/urlcache.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/dlls/wininet/tests/urlcache.c b/dlls/wininet/tests/urlcache.c index b1296d34c7a..1b9c80b8bf3 100644 --- a/dlls/wininet/tests/urlcache.c +++ b/dlls/wininet/tests/urlcache.c @@ -1006,6 +1006,21 @@ static void test_urlcacheW(void) ok(ret, "%ld) DeleteUrlCacheEntryW failed: %ld\n", i, GetLastError()); } } + + /* CreateUrlCacheEntryW has no output buffer size parameter, so it must not + * write more than MAX_PATH characters to lpszFileName. + */ + for(i=0; i<ARRAY_SIZE(bufW) -1; i++) + bufW[i] = 'a'; + bufW[i] = 0; + + ret = CreateUrlCacheEntryW(test_urlW, 0, bufW, bufW, 0); + todo_wine ok(ret, "CreateUrlCacheEntryW failed: %ld\n", GetLastError()); + if(ret) { + ok(lstrlenW(bufW) < MAX_PATH, "cache path too long: %s\n", wine_dbgstr_w(bufW)); + ret = DeleteFileW(bufW); + ok(ret, "DeleteFileW failed: %ld\n", GetLastError()); + } } static void test_FindCloseUrlCache(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11255
From: Jiajin Cui <cuijiajin@uniontech.com> Add coverage for URLDownloadToCacheFileW and URLDownloadToCacheFileA with long URL extensions to verify output buffers are not overrun. Signed-off-by: Jiajin Cui <cuijiajin@uniontech.com> --- dlls/urlmon/tests/misc.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/dlls/urlmon/tests/misc.c b/dlls/urlmon/tests/misc.c index 2a61b892930..1716af1766b 100644 --- a/dlls/urlmon/tests/misc.c +++ b/dlls/urlmon/tests/misc.c @@ -28,6 +28,7 @@ #include "winbase.h" #include "ole2.h" #include "urlmon.h" +#include "wininet.h" #include "initguid.h" @@ -2725,6 +2726,43 @@ static void test_bsc_marshaling(void) TerminateThread(thread, 0); } +static void test_URLDownloadToCacheFile(void) +{ + WCHAR url[512], filename[MAX_PATH]; + static const WCHAR prefix[] = L"http://127.0.0.1:1/test."; + char urlA[ARRAY_SIZE(url)], filenameA[MAX_PATH]; + DWORD size; + HRESULT hres; + BOOL ret; + unsigned int i, len; + + lstrcpyW(url, prefix); + len = lstrlenW(url); + for(i = len; i < ARRAY_SIZE(url) - 1; i++) + url[i] = 'a'; + url[i] = 0; + + memset(filename, 0xcc, sizeof(filename)); + hres = URLDownloadToCacheFileW(NULL, url, filename, ARRAY_SIZE(filename), 0, NULL); + ok(FAILED(hres), "URLDownloadToCacheFileW returned %#lx\n", hres); + ok(filename[ARRAY_SIZE(filename) - 1] == 0xcccc, "filename buffer overflowed\n"); + + WideCharToMultiByte(CP_ACP, 0, url, -1, urlA, sizeof(urlA), NULL, NULL); + size = 0; + ret = GetUrlCacheEntryInfoA(urlA, NULL, &size); + ok(!ret && GetLastError() == ERROR_FILE_NOT_FOUND, "cache entry was not deleted, error %ld\n", GetLastError()); + /* Always clean up any remaining cache entry */ + if (ret || GetLastError() != ERROR_FILE_NOT_FOUND) + DeleteUrlCacheEntryA(urlA); + + memset(filenameA, 0xcc, sizeof(filenameA)); + hres = URLDownloadToCacheFileA(NULL, urlA, filenameA, sizeof(filenameA), 0, NULL); + ok(FAILED(hres), "URLDownloadToCacheFileA returned %#lx\n", hres); + ok(filenameA[sizeof(filenameA) - 1] == (char)0xcc, "filenameA buffer overflowed\n"); + /* Always clean up any remaining cache entry */ + DeleteUrlCacheEntryA(urlA); +} + static void test_MapBrowserEmulationModeToUserAgent(BOOL custom_ua) { /* Undocumented structure of unknown size, crashes if it's too small (with arbitrary values from stack) */ @@ -2832,6 +2870,7 @@ START_TEST(misc) test_MkParseDisplayNameEx(); test_IsValidURL(); test_bsc_marshaling(); + test_URLDownloadToCacheFile(); test_MapBrowserEmulationModeToUserAgent(TRUE); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11255
From: Jiajin Cui <cuijiajin@uniontech.com> Avoid appending cache file extensions that cannot fit in the generated MAX_PATH-sized cache path. Signed-off-by: Jiajin Cui <cuijiajin@uniontech.com> --- dlls/wininet/tests/urlcache.c | 2 +- dlls/wininet/urlcache.c | 26 +++++++++++++++----------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/dlls/wininet/tests/urlcache.c b/dlls/wininet/tests/urlcache.c index 1b9c80b8bf3..0cf86d2a782 100644 --- a/dlls/wininet/tests/urlcache.c +++ b/dlls/wininet/tests/urlcache.c @@ -1015,7 +1015,7 @@ static void test_urlcacheW(void) bufW[i] = 0; ret = CreateUrlCacheEntryW(test_urlW, 0, bufW, bufW, 0); - todo_wine ok(ret, "CreateUrlCacheEntryW failed: %ld\n", GetLastError()); + ok(ret, "CreateUrlCacheEntryW failed: %ld\n", GetLastError()); if(ret) { ok(lstrlenW(bufW) < MAX_PATH, "cache path too long: %s\n", wine_dbgstr_w(bufW)); ret = DeleteFileW(bufW); diff --git a/dlls/wininet/urlcache.c b/dlls/wininet/urlcache.c index 49c8ce9becc..933e33e6834 100644 --- a/dlls/wininet/urlcache.c +++ b/dlls/wininet/urlcache.c @@ -2689,19 +2689,23 @@ static BOOL urlcache_entry_create(const char *url, const char *ext, WCHAR *full_ extW[0] = '.'; ext_len = MultiByteToWideChar(CP_ACP, 0, ext, -1, extW+1, MAX_PATH-1); - - for(p=extW; *p; p++) { - switch(*p) { - case '<': case '>': - case ':': case '"': - case '|': case '?': - case '*': - *p = '_'; break; - default: break; + if(!ext_len || ext_len >= MAX_PATH - 8) { + extW[0] = '\0'; + ext_len = 0; + }else { + for(p=extW; *p; p++) { + switch(*p) { + case '<': case '>': + case ':': case '"': + case '|': case '?': + case '*': + *p = '_'; break; + default: break; + } } + if(p > extW && (p[-1]==' ' || p[-1]=='.')) + p[-1] = '_'; } - if(p[-1]==' ' || p[-1]=='.') - p[-1] = '_'; }else { extW[0] = '\0'; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11255
From: Jiajin Cui <cuijiajin@uniontech.com> Pass the ANSI buffer length in characters and account for the null terminator before copying the cache path. Signed-off-by: Jiajin Cui <cuijiajin@uniontech.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --- dlls/urlmon/umon.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/urlmon/umon.c b/dlls/urlmon/umon.c index 0ee8502d070..f00895f9384 100644 --- a/dlls/urlmon/umon.c +++ b/dlls/urlmon/umon.c @@ -850,7 +850,7 @@ HRESULT WINAPI URLDownloadToCacheFileA(LPUNKNOWN lpUnkCaller, LPCSTR szURL, LPST if(szFileName) file_name = malloc(dwBufLength * sizeof(WCHAR)); - hres = URLDownloadToCacheFileW(lpUnkCaller, url, file_name, dwBufLength*sizeof(WCHAR), + hres = URLDownloadToCacheFileW(lpUnkCaller, url, file_name, dwBufLength, dwReserved, pBSC); if(SUCCEEDED(hres) && file_name) @@ -899,7 +899,7 @@ HRESULT WINAPI URLDownloadToCacheFileW(LPUNKNOWN lpUnkCaller, LPCWSTR szURL, LPW header, sizeof(header), NULL, NULL)) return E_FAIL; - if (lstrlenW(cache_path) > dwBufLength) + if (lstrlenW(cache_path) + 1 > dwBufLength) return E_OUTOFMEMORY; lstrcpyW(szFileName, cache_path); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11255
participants (2)
-
Jiajin Cui -
Jiajin Cui (＠jin-king1)