From 55c9707639c40d78731cfff6ea7aaaddc6e8542a Mon Sep 17 00:00:00 2001 From: fufesou Date: Tue, 12 May 2026 16:24:50 +0800 Subject: [PATCH] fix(msi): check install folder, remove files when uninstall (#15011) * fix(msi): check install folder, remove files when uninstall Signed-off-by: fufesou * fix(msi): harden install folder normalization cleanup Signed-off-by: fufesou * fix(msi): better file attributes Signed-off-by: fufesou * fix(mis): Simple refactor Signed-off-by: fufesou * fix(msi): avoid path-based attribute changes in cleanup Signed-off-by: fufesou * fix(msi): custom action, unset flag read before del Signed-off-by: fufesou --------- Signed-off-by: fufesou --- res/msi/CustomActions/CustomActions.cpp | 169 +++++++++----------- res/msi/CustomActions/CustomActions.def | 2 +- res/msi/Package/Components/Folders.wxs | 11 +- res/msi/Package/Components/RustDesk.wxs | 16 +- res/msi/Package/Fragments/CustomActions.wxs | 2 +- res/msi/Package/UI/MyInstallDlg.wxs | 16 +- 6 files changed, 109 insertions(+), 107 deletions(-) diff --git a/res/msi/CustomActions/CustomActions.cpp b/res/msi/CustomActions/CustomActions.cpp index 0107929f3..f4780dd87 100644 --- a/res/msi/CustomActions/CustomActions.cpp +++ b/res/msi/CustomActions/CustomActions.cpp @@ -31,17 +31,17 @@ LExit: return WcaFinalize(er); } -// Helper function to safely delete a file or directory using handle-based deletion. -// This avoids TOCTOU (Time-Of-Check-Time-Of-Use) race conditions. +// Helper function to safely delete a file using handle-based deletion. +// Directories are refused after opening the handle. BOOL SafeDeleteItem(LPCWSTR fullPath) { - // Open the file/directory with DELETE access and FILE_FLAG_OPEN_REPARSE_POINT + // Open the file/directory with delete and attribute-read access plus FILE_FLAG_OPEN_REPARSE_POINT // to prevent following symlinks. // Use shared access to allow deletion even when other processes have the file open. DWORD flags = FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT; HANDLE hFile = CreateFileW( fullPath, - DELETE, + DELETE | FILE_READ_ATTRIBUTES, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, // Allow shared access NULL, OPEN_EXISTING, @@ -55,6 +55,21 @@ BOOL SafeDeleteItem(LPCWSTR fullPath) return FALSE; } + BY_HANDLE_FILE_INFORMATION fileInfo; + if (FALSE == GetFileInformationByHandle(hFile, &fileInfo)) + { + WcaLog(LOGMSG_STANDARD, "SafeDeleteItem: Failed to inspect '%ls'. Error: %lu", fullPath, GetLastError()); + CloseHandle(hFile); + return FALSE; + } + + if (fileInfo.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) + { + WcaLog(LOGMSG_STANDARD, "SafeDeleteItem: Refusing to delete directory '%ls'.", fullPath); + CloseHandle(hFile); + return FALSE; + } + // Use SetFileInformationByHandle to mark for deletion. // The file will be deleted when the handle is closed. FILE_DISPOSITION_INFO dispInfo; @@ -77,98 +92,74 @@ BOOL SafeDeleteItem(LPCWSTR fullPath) return result; } -// Helper function to recursively delete a directory's contents with detailed logging. -void RecursiveDelete(LPCWSTR path) +BOOL PathEndsWithSlash(LPCWSTR path) { - // Ensure the path is not empty or null. - if (path == NULL || path[0] == L'\0') + size_t length = 0; + HRESULT hr = StringCchLengthW(path, MAX_PATH, &length); + if (FAILED(hr) || length == 0) + { + return FALSE; + } + + WCHAR last = path[length - 1]; + return last == L'\\' || last == L'/'; +} + +void ClearReadOnlyAttribute(LPCWSTR fullPath, DWORD attributes) +{ + if (!(attributes & FILE_ATTRIBUTE_READONLY)) { return; } - // Extra safety: never operate directly on a root path. - if (PathIsRootW(path)) + DWORD writableAttributes = attributes & ~FILE_ATTRIBUTE_READONLY; + if (writableAttributes == 0) { - WcaLog(LOGMSG_STANDARD, "RecursiveDelete: refusing to operate on root path '%ls'.", path); + writableAttributes = FILE_ATTRIBUTE_NORMAL; + } + + if (SetFileAttributesW(fullPath, writableAttributes)) + { + WcaLog(LOGMSG_STANDARD, "Runtime cleanup cleared read-only attribute for '%ls'.", fullPath); return; } - // MAX_PATH is enough here since the installer should not be using longer paths. - // No need to handle extended-length paths (\\?\) in this context. - WCHAR searchPath[MAX_PATH]; - HRESULT hr = StringCchPrintfW(searchPath, MAX_PATH, L"%s\\*", path); - if (FAILED(hr)) { - WcaLog(LOGMSG_STANDARD, "RecursiveDelete: Path too long to enumerate: %ls", path); - return; + WcaLog(LOGMSG_STANDARD, "Runtime cleanup failed to clear read-only attribute for '%ls'. Error: %lu", fullPath, GetLastError()); +} + +BOOL DeleteRuntimeGeneratedFile(LPCWSTR installFolder, LPCWSTR fileName) +{ + WCHAR fullPath[MAX_PATH]; + LPCWSTR separator = PathEndsWithSlash(installFolder) ? L"" : L"\\"; + HRESULT hr = StringCchPrintfW(fullPath, MAX_PATH, L"%s%s%s", installFolder, separator, fileName); + if (FAILED(hr)) + { + WcaLog(LOGMSG_STANDARD, "Runtime cleanup path is too long for '%ls'.", fileName); + return FALSE; } - WIN32_FIND_DATAW findData; - HANDLE hFind = FindFirstFileW(searchPath, &findData); - - if (hFind == INVALID_HANDLE_VALUE) + DWORD attributes = GetFileAttributesW(fullPath); + if (attributes == INVALID_FILE_ATTRIBUTES) { - // This can happen if the directory is empty or doesn't exist, which is not an error in our case. - WcaLog(LOGMSG_STANDARD, "RecursiveDelete: Failed to enumerate directory '%ls'. It may be missing or inaccessible. Error: %lu", path, GetLastError()); - return; + DWORD error = GetLastError(); + if (error == ERROR_FILE_NOT_FOUND || error == ERROR_PATH_NOT_FOUND) + { + return TRUE; + } + + WcaLog(LOGMSG_STANDARD, "Runtime cleanup cannot stat '%ls'. Error: %lu", fullPath, error); + return FALSE; } - do + if (attributes & FILE_ATTRIBUTE_DIRECTORY) { - // Skip '.' and '..' directories. - if (wcscmp(findData.cFileName, L".") == 0 || wcscmp(findData.cFileName, L"..") == 0) - { - continue; - } - - // MAX_PATH is enough here since the installer should not be using longer paths. - // No need to handle extended-length paths (\\?\) in this context. - WCHAR fullPath[MAX_PATH]; - hr = StringCchPrintfW(fullPath, MAX_PATH, L"%s\\%s", path, findData.cFileName); - if (FAILED(hr)) { - WcaLog(LOGMSG_STANDARD, "RecursiveDelete: Path too long for item '%ls' in '%ls', skipping.", findData.cFileName, path); - continue; - } - - // Before acting, ensure the read-only attribute is not set. - if (findData.dwFileAttributes & FILE_ATTRIBUTE_READONLY) - { - if (FALSE == SetFileAttributesW(fullPath, findData.dwFileAttributes & ~FILE_ATTRIBUTE_READONLY)) - { - WcaLog(LOGMSG_STANDARD, "RecursiveDelete: Failed to remove read-only attribute. Error: %lu", GetLastError()); - } - } - - if (findData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) - { - // Check for reparse points (symlinks/junctions) to prevent directory traversal attacks. - // Do not follow reparse points, only remove the link itself. - if (findData.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) - { - WcaLog(LOGMSG_STANDARD, "RecursiveDelete: Not recursing into reparse point (symlink/junction), deleting link itself: %ls", fullPath); - SafeDeleteItem(fullPath); - } - else - { - // Recursively delete directory contents first - RecursiveDelete(fullPath); - // Then delete the directory itself - SafeDeleteItem(fullPath); - } - } - else - { - // Delete file using safe handle-based deletion - SafeDeleteItem(fullPath); - } - } while (FindNextFileW(hFind, &findData) != 0); - - DWORD lastError = GetLastError(); - if (lastError != ERROR_NO_MORE_FILES) - { - WcaLog(LOGMSG_STANDARD, "RecursiveDelete: FindNextFileW failed with error %lu", lastError); + WcaLog(LOGMSG_STANDARD, "Runtime cleanup skipped directory '%ls'.", fullPath); + return FALSE; } - FindClose(hFind); + ClearReadOnlyAttribute(fullPath, attributes); + WcaLog(LOGMSG_STANDARD, "Runtime cleanup deleting '%ls'.", fullPath); + return SafeDeleteItem(fullPath); } // See `Package.wxs` for the sequence of this custom action. @@ -178,13 +169,13 @@ void RecursiveDelete(LPCWSTR path) // 2. RemoveExistingProducts // ├─ TerminateProcesses // ├─ TryStopDeleteService -// ├─ RemoveInstallFolder - <-- Here +// ├─ RemoveRuntimeGeneratedFiles - <-- Here // └─ RemoveFiles // 3. InstallValidate // 4. InstallFiles // 5. InstallExecute // 6. InstallFinalize -UINT __stdcall RemoveInstallFolder( +UINT __stdcall RemoveRuntimeGeneratedFiles( __in MSIHANDLE hInstall) { HRESULT hr = S_OK; @@ -194,7 +185,7 @@ UINT __stdcall RemoveInstallFolder( LPWSTR pwz = NULL; LPWSTR pwzData = NULL; - hr = WcaInitialize(hInstall, "RemoveInstallFolder"); + hr = WcaInitialize(hInstall, "RemoveRuntimeGeneratedFiles"); ExitOnFailure(hr, "Failed to initialize"); hr = WcaGetProperty(L"CustomActionData", &pwzData); @@ -202,24 +193,20 @@ UINT __stdcall RemoveInstallFolder( pwz = pwzData; hr = WcaReadStringFromCaData(&pwz, &installFolder); - ExitOnFailure(hr, "failed to read database key from custom action data: %ls", pwz); + ExitOnFailure(hr, "failed to read install folder from custom action data: %ls", pwz); if (installFolder == NULL || installFolder[0] == L'\0') { - WcaLog(LOGMSG_STANDARD, "Install folder path is empty, skipping recursive delete."); + WcaLog(LOGMSG_STANDARD, "Install folder path is empty, skipping runtime cleanup."); goto LExit; } if (PathIsRootW(installFolder)) { - WcaLog(LOGMSG_STANDARD, "Refusing to recursively delete root folder '%ls'.", installFolder); + WcaLog(LOGMSG_STANDARD, "Refusing runtime cleanup in root folder '%ls'.", installFolder); goto LExit; } - WcaLog(LOGMSG_STANDARD, "Attempting to recursively delete contents of install folder: %ls", installFolder); - - RecursiveDelete(installFolder); - - // The standard MSI 'RemoveFolders' action will take care of removing the (now empty) directories. - // We don't need to call RemoveDirectoryW on installFolder itself, as it might still be in use by the installer. + WcaLog(LOGMSG_STANDARD, "Removing runtime-generated files from install folder: %ls", installFolder); + DeleteRuntimeGeneratedFile(installFolder, L"RuntimeBroker_rustdesk.exe"); LExit: ReleaseStr(pwzData); diff --git a/res/msi/CustomActions/CustomActions.def b/res/msi/CustomActions/CustomActions.def index 01b03490c..d50fbf59b 100644 --- a/res/msi/CustomActions/CustomActions.def +++ b/res/msi/CustomActions/CustomActions.def @@ -2,7 +2,7 @@ LIBRARY "CustomActions" EXPORTS CustomActionHello - RemoveInstallFolder + RemoveRuntimeGeneratedFiles TerminateProcesses AddFirewallRules SetPropertyIsServiceRunning diff --git a/res/msi/Package/Components/Folders.wxs b/res/msi/Package/Components/Folders.wxs index de9edb7f3..6911600e9 100644 --- a/res/msi/Package/Components/Folders.wxs +++ b/res/msi/Package/Components/Folders.wxs @@ -16,8 +16,15 @@ - - + + + + + + + + + diff --git a/res/msi/Package/Components/RustDesk.wxs b/res/msi/Package/Components/RustDesk.wxs index 337e84ec3..952172bdc 100644 --- a/res/msi/Package/Components/RustDesk.wxs +++ b/res/msi/Package/Components/RustDesk.wxs @@ -12,7 +12,7 @@ - + @@ -77,21 +77,21 @@ - - - + + + - + - + - + - + diff --git a/res/msi/Package/Fragments/CustomActions.wxs b/res/msi/Package/Fragments/CustomActions.wxs index 3727c0dd3..3a9811eb8 100644 --- a/res/msi/Package/Fragments/CustomActions.wxs +++ b/res/msi/Package/Fragments/CustomActions.wxs @@ -5,7 +5,7 @@ - + diff --git a/res/msi/Package/UI/MyInstallDlg.wxs b/res/msi/Package/UI/MyInstallDlg.wxs index bf59d569c..06c37097c 100644 --- a/res/msi/Package/UI/MyInstallDlg.wxs +++ b/res/msi/Package/UI/MyInstallDlg.wxs @@ -23,12 +23,13 @@ Patch dialog sequence: --> + - + @@ -64,9 +65,16 @@ Patch dialog sequence: - - - + + + + + + + + + +