diff --git a/Sources/System/FilePath/FilePathTempWindows.swift b/Sources/System/FilePath/FilePathTempWindows.swift index 0321d524..0de6dcc0 100644 --- a/Sources/System/FilePath/FilePathTempWindows.swift +++ b/Sources/System/FilePath/FilePathTempWindows.swift @@ -35,7 +35,7 @@ internal func _getTemporaryDirectory() throws -> FilePath { fileprivate func forEachFile( at path: FilePath, _ body: (WIN32_FIND_DATAW) throws -> () -) rethrows { +) throws { let searchPath = path.appending("\\*") try searchPath.withPlatformString { szPath in @@ -59,6 +59,15 @@ fileprivate func forEachFile( try body(findData) } while FindNextFileW(hFind, &findData) + + // FindNextFileW returns false both at the end of the enumeration and on + // error; only ERROR_NO_MORE_FILES is the normal terminator. Treating a + // transient error as end-of-directory would silently skip the remaining + // entries and leave the tree partially deleted. + let error = GetLastError() + if error != ERROR_NO_MORE_FILES { + throw Errno(windowsError: error) + } } } @@ -73,14 +82,28 @@ internal func _recursiveRemove( ) throws { // First, deal with subdirectories try forEachFile(at: path) { findData in - if (findData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0 { - let name = withUnsafeBytes(of: findData.cFileName) { - return SystemString(platformString: $0.assumingMemoryBound( - to: CInterop.PlatformChar.self).baseAddress!) - } - let component = FilePath.Component(name)! - let subpath = path.appending(component) + guard (findData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0 else { + return + } + + let name = withUnsafeBytes(of: findData.cFileName) { + return SystemString(platformString: $0.assumingMemoryBound( + to: CInterop.PlatformChar.self).baseAddress!) + } + let component = FilePath.Component(name)! + let subpath = path.appending(component) + // A directory that is also a reparse point (a junction or directory + // symlink) must not be recursed into: enumerating it would traverse into + // the *target* and delete its contents. Remove the link itself instead; + // RemoveDirectoryW deletes the reparse point without touching the target. + if (findData.dwFileAttributes & DWORD(FILE_ATTRIBUTE_REPARSE_POINT)) != 0 { + try subpath.withPlatformString { subpath in + if try !subpath.withCanonicalPathRepresentation({ RemoveDirectoryW($0) }) { + throw Errno(windowsError: GetLastError()) + } + } + } else { try _recursiveRemove(at: subpath) } } diff --git a/Tests/SystemTests/FilePathTests/FilePathTempTest.swift b/Tests/SystemTests/FilePathTests/FilePathTempTest.swift index 30d58261..815fc13f 100644 --- a/Tests/SystemTests/FilePathTests/FilePathTempTest.swift +++ b/Tests/SystemTests/FilePathTests/FilePathTempTest.swift @@ -8,6 +8,7 @@ */ import XCTest +import Foundation #if SYSTEM_PACKAGE @testable import SystemPackage @@ -15,6 +16,10 @@ import XCTest @testable import System #endif +#if os(Windows) +import WinSDK +#endif + final class TemporaryPathTest: XCTestCase { #if SYSTEM_PACKAGE_DARWIN func testNotInSlashTmp() throws { @@ -55,4 +60,62 @@ final class TemporaryPathTest: XCTestCase { XCTAssertEqual(error as! Errno, Errno.noSuchFileOrDirectory) } } + + #if os(Windows) + /// Recursive removal must not follow directory reparse points (junctions or + /// directory symlinks). Enumerating one traverses into its *target*, so a + /// naive recursion would delete files that live outside the tree being + /// removed. + func testCleanupDoesNotFollowReparsePoints() throws { + func createDirectory(_ path: FilePath) throws { + try path.withPlatformString { + if CreateDirectoryW($0, nil) == false { + throw Errno(windowsError: GetLastError()) + } + } + } + + let tmp = try _getTemporaryDirectory() + let victim = tmp.appending("ss-reparse-victim") + let container = tmp.appending("ss-reparse-container") + // Start from a clean slate and always clean up afterwards. + try? _recursiveRemove(at: victim) + try? _recursiveRemove(at: container) + defer { + try? _recursiveRemove(at: victim) + try? _recursiveRemove(at: container) + } + + // A file that lives outside the tree we are about to remove, and so must + // survive it. + try createDirectory(victim) + let victimFile = victim.appending("keep.txt") + let vfd = try FileDescriptor.open(victimFile, .readWrite, + options: [.create, .truncate], + permissions: .ownerReadWrite) + try vfd.closeAfter { try vfd.writeAll("keep".utf8) } + + // A directory junction inside the container, pointing at the victim. + // A junction (mklink /J) is a reparse point that, unlike a symlink, needs + // no special privilege, so this exercises the reparse-point path in CI too. + try createDirectory(container) + let link = container.appending("link") + let mklink = Process() + mklink.executableURL = URL(fileURLWithPath: "C:\\Windows\\System32\\cmd.exe") + mklink.arguments = ["/c", "mklink", "/J", + String(decoding: link), String(decoding: victim)] + mklink.standardOutput = nil + mklink.standardError = nil + try mklink.run() + mklink.waitUntilExit() + try XCTSkipUnless(mklink.terminationStatus == 0, + "could not create a directory junction") + + // Removing the container must delete the link, not what it points at. + try _recursiveRemove(at: container) + + let reopened = try FileDescriptor.open(victimFile, .readOnly) + try reopened.close() + } + #endif }