Skip to content

Do not follow reparse points when removing a directory tree on Windows - #349

Open
YayoRazo wants to merge 1 commit into
apple:mainfrom
YayoRazo:fix/windows-recursive-temp-removal
Open

Do not follow reparse points when removing a directory tree on Windows#349
YayoRazo wants to merge 1 commit into
apple:mainfrom
YayoRazo:fix/windows-recursive-temp-removal

Conversation

@YayoRazo

Copy link
Copy Markdown

Summary

_recursiveRemove (used by withTemporaryFilePath cleanup on Windows) enumerated a directory's children and recursed into any entry with FILE_ATTRIBUTE_DIRECTORY set. A directory that is also a reparse point — a junction or a directory symlink — carries that attribute, so the routine recursed through the link and deleted the contents of its target, which lives outside the tree being removed.

For a temporary directory that happens to contain a junction pointing at, say, the user's documents, this was silent data loss: the real files behind the link were deleted, not the link.

Fix

  • Skip reparse points during recursion: remove the link itself with RemoveDirectoryW, which deletes the reparse point without touching its target. This matches the POSIX sibling (FilePathTempPosix.swift), which only recurses on real directories (DT_DIR) and never on symlinks.
  • Harden the enumeration in forEachFile: FindNextFileW returns false both at the end of a directory and on error, so the loop now verifies it ended with ERROR_NO_MORE_FILES rather than treating a transient failure as end-of-directory (which would silently skip the remaining entries and leave the tree partially deleted). forEachFile becomes throws instead of rethrows.

Testing

  • Added testCleanupDoesNotFollowReparsePoints: it creates a file in an external "victim" directory, places a directory junction to it inside the tree being removed (via mklink /J, which needs no special privilege, so the reparse path is exercised in CI), removes the tree, and asserts the victim file survives.
  • Verified the test fails without this change (the victim file is deleted through the junction) and passes with it.
  • Full swift test suite green on Windows (x86_64-unknown-windows-msvc, Swift 6.3.3); no changes to other platforms.

`_recursiveRemove` enumerated a directory's children and recursed into any
entry with `FILE_ATTRIBUTE_DIRECTORY` set. A directory that is also a reparse
point — a junction or a directory symlink — carries that attribute, so the
routine recursed *through* the link and deleted the contents of its target,
which lives outside the tree being removed. For a temporary directory holding
a junction to, say, the user's documents, this was silent data loss.

Skip reparse points instead: remove the link itself with RemoveDirectoryW,
which deletes the reparse point without touching the target. This matches the
POSIX sibling, which only recurses on real directories.

While here, harden the enumeration in `forEachFile`: `FindNextFileW` returns
false both at the end of a directory and on error, so check that the loop
ended with ERROR_NO_MORE_FILES rather than treating a transient failure as
end-of-directory (which would silently skip entries and leave the tree
behind). This makes `forEachFile` `throws` rather than `rethrows`.
at path: FilePath,
_ body: (WIN32_FIND_DATAW) throws -> ()
) rethrows {
) throws {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a spurious change; this should still be rethrows until we change forEachFile() to throw an Errno (or an Error-constrained type parameter).

Comment on lines +90 to +91
return SystemString(platformString: $0.assumingMemoryBound(
to: CInterop.PlatformChar.self).baseAddress!)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's replace this with memory rebinding. The older code likely predated the availability of SE-0333.

Suggested change
return SystemString(platformString: $0.assumingMemoryBound(
to: CInterop.PlatformChar.self).baseAddress!)
$0.withMemoryRebound(to: CInterop.PlatformChar.self) {
SystemString(platformString: $0.baseAddress!)
}

@glessard

Copy link
Copy Markdown
Contributor

This is indeed a bug, but it only involves an internal utility function. We can take our time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants