diff --git a/include/wil/filesystem.h b/include/wil/filesystem.h index 449564e2..781d7e69 100644 --- a/include/wil/filesystem.h +++ b/include/wil/filesystem.h @@ -200,6 +200,8 @@ enum class RemoveDirectoryOptions None = 0, KeepRootDirectory = 0x1, RemoveReadOnly = 0x2, + RemoveReadOnlyFile = RemoveReadOnly, + RemoveReadOnlyDirectory = 0x4, }; DEFINE_ENUM_FLAG_OPERATORS(RemoveDirectoryOptions); @@ -315,9 +317,9 @@ inline HRESULT RemoveDirectoryRecursiveNoThrow( // Try a DeleteFile. Some errors may be recoverable. if (!::DeleteFileW(pathToDelete.get())) { - // Fail for anything other than ERROR_ACCESS_DENIED with option to RemoveReadOnly available + // Fail for anything other than ERROR_ACCESS_DENIED with option to RemoveReadOnlyFile available bool potentiallyFixableReadOnlyProblem = - WI_IsFlagSet(options, RemoveDirectoryOptions::RemoveReadOnly) && ::GetLastError() == ERROR_ACCESS_DENIED; + WI_IsFlagSet(options, RemoveDirectoryOptions::RemoveReadOnlyFile) && ::GetLastError() == ERROR_ACCESS_DENIED; RETURN_LAST_ERROR_IF(!potentiallyFixableReadOnlyProblem); // Fail if the file does not have read-only set, likely just an ACL problem @@ -378,7 +380,29 @@ inline HRESULT RemoveDirectoryRecursiveNoThrow( } else { - RETURN_IF_WIN32_BOOL_FALSE(::RemoveDirectoryW(path.get())); + // Try a RemoveDirectory. Some errors may be recoverable. + if (!::RemoveDirectoryW(path.get())) + { + // Fail for anything other than ERROR_ACCESS_DENIED with option to RemoveReadOnlyDirectory available + bool potentiallyFixableReadOnlyProblem = + WI_IsFlagSet(options, RemoveDirectoryOptions::RemoveReadOnlyDirectory) && ::GetLastError() == ERROR_ACCESS_DENIED; + RETURN_LAST_ERROR_IF(!potentiallyFixableReadOnlyProblem); + + // Fail if the directory does not have read-only set, likely just an ACL problem + DWORD directoryAttr = ::GetFileAttributesW(path.get()); + RETURN_LAST_ERROR_IF(!WI_IsFlagSet(directoryAttr, FILE_ATTRIBUTE_READONLY)); + + // Remove read-only flag, setting to NORMAL if completely empty + WI_ClearFlag(directoryAttr, FILE_ATTRIBUTE_READONLY); + if (directoryAttr == 0) + { + directoryAttr = FILE_ATTRIBUTE_DIRECTORY; + } + + // Set the new attributes and try to delete the directory again, returning any failure + ::SetFileAttributesW(path.get(), directoryAttr); + RETURN_IF_WIN32_BOOL_FALSE(::RemoveDirectoryW(path.get())); + } } } return S_OK; diff --git a/tests/FileSystemTests.cpp b/tests/FileSystemTests.cpp index e63823e8..63872947 100644 --- a/tests/FileSystemTests.cpp +++ b/tests/FileSystemTests.cpp @@ -127,7 +127,7 @@ TEST_CASE("FileSystemTests::VerifyRemoveDirectoryRecursiveDoesNotTraverseWithout subFolderHandle.reset(); } -TEST_CASE("FileSystemTests::VerifyRemoveDirectoryRecursiveCanDeleteReadOnlyFiles", "[filesystem]") +TEST_CASE("FileSystemTests::VerifyRemoveDirectoryRecursiveCanDeleteReadOnlyFilesAndDirectories", "[filesystem]") { auto CreateRelativePath = [](PCWSTR root, PCWSTR name) { wil::unique_hlocal_string path; @@ -140,37 +140,54 @@ TEST_CASE("FileSystemTests::VerifyRemoveDirectoryRecursiveCanDeleteReadOnlyFiles REQUIRE(fileHandle); }; + auto CreateReadOnlyDirectory = [](PCWSTR path) { + REQUIRE(::CreateDirectoryW(path, nullptr)); + DWORD directoryAttr = ::GetFileAttributesW(path); + REQUIRE(::SetFileAttributesW(path, directoryAttr | FILE_ATTRIBUTE_READONLY)); + }; + wil::unique_cotaskmem_string tempPath; REQUIRE_SUCCEEDED(wil::ExpandEnvironmentStringsW(LR"(%TEMP%)", tempPath)); const auto basePath = CreateRelativePath(tempPath.get(), L"FileSystemTests"); REQUIRE_SUCCEEDED(wil::CreateDirectoryDeepNoThrow(basePath.get())); auto scopeGuard = wil::scope_exit([&] { - wil::RemoveDirectoryRecursiveNoThrow(basePath.get(), wil::RemoveDirectoryOptions::RemoveReadOnly); + wil::RemoveDirectoryRecursiveNoThrow( + basePath.get(), wil::RemoveDirectoryOptions::RemoveReadOnlyFile | wil::RemoveDirectoryOptions::RemoveReadOnlyDirectory); }); // Create a reparse point and a target folder that shouldn't get deleted auto folderToDelete = CreateRelativePath(basePath.get(), L"folderToDelete"); REQUIRE(::CreateDirectoryW(folderToDelete.get(), nullptr)); - auto topLevelReadOnly = CreateRelativePath(folderToDelete.get(), L"topLevelReadOnly.txt"); - CreateReadOnlyFile(topLevelReadOnly.get()); + auto topLevelReadOnlyDirectory = CreateRelativePath(folderToDelete.get(), L"topLevelReadOnly"); + CreateReadOnlyDirectory(topLevelReadOnlyDirectory.get()); + + auto topLevelReadOnlyFile = CreateRelativePath(folderToDelete.get(), L"topLevelReadOnly.txt"); + CreateReadOnlyFile(topLevelReadOnlyFile.get()); auto subLevel = CreateRelativePath(folderToDelete.get(), L"subLevel"); REQUIRE(::CreateDirectoryW(subLevel.get(), nullptr)); - auto subLevelReadOnly = CreateRelativePath(subLevel.get(), L"subLevelReadOnly.txt"); - CreateReadOnlyFile(subLevelReadOnly.get()); + auto subLevelReadOnlyDirectory = CreateRelativePath(subLevel.get(), L"subLevelReadOnly"); + CreateReadOnlyDirectory(subLevelReadOnlyDirectory.get()); - // Delete will fail without the RemoveReadOnlyFlag + auto subLevelReadOnlyFile = CreateRelativePath(subLevel.get(), L"subLevelReadOnly.txt"); + CreateReadOnlyFile(subLevelReadOnlyFile.get()); + + // Delete will fail without the RemoveReadOnlyFile | RemoveReadOnlyDirectory flags REQUIRE_FAILED(wil::RemoveDirectoryRecursiveNoThrow(folderToDelete.get())); - REQUIRE_SUCCEEDED(wil::RemoveDirectoryRecursiveNoThrow(folderToDelete.get(), wil::RemoveDirectoryOptions::RemoveReadOnly)); + REQUIRE_SUCCEEDED( + wil::RemoveDirectoryRecursiveNoThrow( + folderToDelete.get(), wil::RemoveDirectoryOptions::RemoveReadOnlyFile | wil::RemoveDirectoryOptions::RemoveReadOnlyDirectory)); // Verify all files have been deleted - REQUIRE_FALSE(FileExists(subLevelReadOnly.get())); + REQUIRE_FALSE(FileExists(subLevelReadOnlyFile.get())); + REQUIRE_FALSE(DirectoryExists(subLevelReadOnlyDirectory.get())); REQUIRE_FALSE(DirectoryExists(subLevel.get())); - REQUIRE_FALSE(FileExists(topLevelReadOnly.get())); + REQUIRE_FALSE(FileExists(topLevelReadOnlyFile.get())); + REQUIRE_FALSE(DirectoryExists(topLevelReadOnlyDirectory.get())); REQUIRE_FALSE(DirectoryExists(folderToDelete.get())); }