[PATCH 0/2] MR11331: urlmon: Fix temporary cache file leak in URLDownloadToCacheFileW.
URLDownloadToCacheFileW calls CreateUrlCacheEntryW to allocate a temporary cache file before downloading the URL into it. When the download is aborted or the subsequent CommitUrlCacheEntryW call fails, the function returned early without removing that temporary file, leaving a stale zero-byte entry behind in the cache directory on every failure. Signed-off-by: Jiajin Cui <cuijiajin@uniontech.com> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11331
From: Jiajin Cui <cuijiajin@uniontech.com> Add a test verifying that URLDownloadToCacheFileW does not leak the temporary cache file when the download is aborted. Cache entries are distributed across randomly chosen sub-directories, so the test counts matching files across all cache sub-directories before and after the aborted download instead of relying on a predicted path. Signed-off-by: Jiajin Cui <cuijiajin@uniontech.com> --- dlls/urlmon/tests/url.c | 114 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/dlls/urlmon/tests/url.c b/dlls/urlmon/tests/url.c index 16742d20ff7..66c1725cdeb 100644 --- a/dlls/urlmon/tests/url.c +++ b/dlls/urlmon/tests/url.c @@ -3746,6 +3746,117 @@ static void test_URLDownloadToFile_abort(void) DeleteFileA(dwl_htmlA); } +static DWORD count_cache_files(const WCHAR *cache_root, const WCHAR *prefix) +{ + WCHAR pattern[MAX_PATH], sub_pattern[MAX_PATH]; + WIN32_FIND_DATAW dir_data, data; + HANDLE dir_find, find; + DWORD count = 0; + + /* Cache entries are randomly distributed across sub-directories, so scan + * every sub-directory of the cache root for files matching the prefix. */ + lstrcpyW(pattern, cache_root); + lstrcatW(pattern, L"\\*"); + + dir_find = FindFirstFileW(pattern, &dir_data); + if(dir_find == INVALID_HANDLE_VALUE) + return 0; + + do { + if(!(dir_data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)) + continue; + if(!lstrcmpW(dir_data.cFileName, L".") || !lstrcmpW(dir_data.cFileName, L"..")) + continue; + + lstrcpyW(sub_pattern, cache_root); + lstrcatW(sub_pattern, L"\\"); + lstrcatW(sub_pattern, dir_data.cFileName); + lstrcatW(sub_pattern, L"\\"); + lstrcatW(sub_pattern, prefix); + lstrcatW(sub_pattern, L"*"); + + find = FindFirstFileW(sub_pattern, &data); + if(find != INVALID_HANDLE_VALUE) { + do { + count++; + } while(FindNextFileW(find, &data)); + FindClose(find); + } + } while(FindNextFileW(dir_find, &dir_data)); + + FindClose(dir_find); + return count; +} + +static void test_URLDownloadToCacheFile_abort(void) +{ + WCHAR cache_file[MAX_PATH], cache_root[MAX_PATH], file_name[MAX_PATH]; + WCHAR url[INTERNET_MAX_URL_LENGTH]; + DWORD before, after; + HRESULT hres; + BOOL res; + + wsprintfW(url, L"http://test.winehq.org/tests/urlmon-cache-cleanup-%lu.txt", GetCurrentProcessId()); + + /* CreateUrlCacheEntryW places the file in a randomly chosen cache + * sub-directory, so the path used internally by URLDownloadToCacheFileW + * cannot be predicted. Remove the placeholder created here and compare the + * number of matching files across all cache sub-directories before and + * after the aborted download to detect a leaked temporary file. */ + res = CreateUrlCacheEntryW(url, 0, PathFindExtensionW(url), cache_file, 0); + ok(res, "CreateUrlCacheEntryW failed: %lu\n", GetLastError()); + if (!res) + return; + + lstrcpyW(cache_root, cache_file); + PathRemoveFileSpecW(cache_root); /* strip file name -> sub-directory */ + PathRemoveFileSpecW(cache_root); /* strip sub-directory -> cache root */ + + res = DeleteFileW(cache_file); + ok(res, "DeleteFileW failed: %lu\n", GetLastError()); + + before = count_cache_files(cache_root, L"urlmon-cache-cleanup-"); + + init_bind_test(HTTP_TEST, BINDTEST_FILEDWLAPI|BINDTEST_ABORT_PROGRESS, TYMED_FILE); + lstrcpyW(current_url, url); + + SET_EXPECT(GetBindInfo); + SET_EXPECT(QueryInterface_IInternetProtocol); + SET_EXPECT(QueryInterface_IServiceProvider); + SET_EXPECT(QueryService_IInternetProtocol); + SET_EXPECT(OnStartBinding); + SET_EXPECT(QueryInterface_IHttpNegotiate); + SET_EXPECT(QueryInterface_IHttpNegotiate2); + SET_EXPECT(BeginningTransaction); + SET_EXPECT(GetRootSecurityId); + SET_EXPECT(QueryInterface_IWindowForBindingUI); + SET_EXPECT(OnProgress_CONNECTING); + SET_EXPECT(OnProgress_SENDINGREQUEST); + SET_EXPECT(OnStopBinding); + + hres = URLDownloadToCacheFileW(NULL, current_url, file_name, ARRAY_SIZE(file_name), 0, + (IBindStatusCallback*)&bsc); + ok(hres == E_ABORT, "URLDownloadToCacheFileW failed: %08lx, expected E_ABORT\n", hres); + + CHECK_CALLED(GetBindInfo); + CHECK_CALLED(QueryInterface_IInternetProtocol); + CHECK_CALLED(QueryInterface_IServiceProvider); + CHECK_CALLED(QueryService_IInternetProtocol); + CHECK_CALLED(OnStartBinding); + CHECK_CALLED(QueryInterface_IHttpNegotiate); + CHECK_CALLED(QueryInterface_IHttpNegotiate2); + CHECK_CALLED(BeginningTransaction); + CHECK_CALLED(GetRootSecurityId); + CLEAR_CALLED(QueryInterface_IWindowForBindingUI); + CHECK_CALLED(OnProgress_SENDINGREQUEST); + CLEAR_CALLED(OnProgress_CONNECTING); + CHECK_CALLED(OnStopBinding); + + after = count_cache_files(cache_root, L"urlmon-cache-cleanup-"); + todo_wine ok(after == before, "temporary cache file leaked: %lu files before, %lu after\n", + before, after); +} + static void set_file_url(char *path) { CHAR file_urlA[INTERNET_MAX_URL_LENGTH]; @@ -4213,6 +4324,9 @@ START_TEST(url) trace("test URLDownloadToFile abort...\n"); test_URLDownloadToFile_abort(); + trace("test URLDownloadToCacheFile abort...\n"); + test_URLDownloadToCacheFile_abort(); + trace("test emulated http abort...\n"); test_BindToStorage(HTTP_TEST, BINDTEST_EMULATE|BINDTEST_ABORT, TYMED_ISTREAM); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11331
From: Jiajin Cui <cuijiajin@uniontech.com> Remove temporary cache files when download or cache commit fails. Signed-off-by: Jiajin Cui <cuijiajin@uniontech.com> --- dlls/urlmon/tests/url.c | 2 +- dlls/urlmon/umon.c | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/dlls/urlmon/tests/url.c b/dlls/urlmon/tests/url.c index 66c1725cdeb..8e55345e057 100644 --- a/dlls/urlmon/tests/url.c +++ b/dlls/urlmon/tests/url.c @@ -3853,7 +3853,7 @@ static void test_URLDownloadToCacheFile_abort(void) CHECK_CALLED(OnStopBinding); after = count_cache_files(cache_root, L"urlmon-cache-cleanup-"); - todo_wine ok(after == before, "temporary cache file leaked: %lu files before, %lu after\n", + ok(after == before, "temporary cache file leaked: %lu files before, %lu after\n", before, after); } diff --git a/dlls/urlmon/umon.c b/dlls/urlmon/umon.c index f00895f9384..8f3fa892d83 100644 --- a/dlls/urlmon/umon.c +++ b/dlls/urlmon/umon.c @@ -887,8 +887,10 @@ HRESULT WINAPI URLDownloadToCacheFileW(LPUNKNOWN lpUnkCaller, LPCWSTR szURL, LPW return E_FAIL; hr = URLDownloadToFileW(lpUnkCaller, szURL, cache_path, 0, pBSC); - if (FAILED(hr)) + if (FAILED(hr)) { + DeleteFileW(cache_path); return hr; + } expire.dwHighDateTime = 0; expire.dwLowDateTime = 0; @@ -896,8 +898,10 @@ HRESULT WINAPI URLDownloadToCacheFileW(LPUNKNOWN lpUnkCaller, LPCWSTR szURL, LPW modified.dwLowDateTime = 0; if (!CommitUrlCacheEntryW(szURL, cache_path, expire, modified, NORMAL_CACHE_ENTRY, - header, sizeof(header), NULL, NULL)) + header, sizeof(header), NULL, NULL)) { + DeleteFileW(cache_path); return E_FAIL; + } if (lstrlenW(cache_path) + 1 > dwBufLength) return E_OUTOFMEMORY; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11331
@jacek Hello, regarding the patch for deleting files that I mentioned last time in https://gitlab.winehq.org/wine/wine/-/merge_requests/11255, I have added new tests and submitted it again. Could you please review it and let me know if there is anything that needs improvement? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11331#note_145024
The fix looks good to me, thanks. While I appreciate the effort on the test, it seems a bit racy. If another process is using the cache, it could cause flaky results. In this case, I think it would be fine to merge the fix without the test. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11331#note_145061
participants (3)
-
Jacek Caban (＠jacek) -
Jiajin Cui
-
Jiajin Cui (＠jin-king1)