Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve path normalization and judgment on Windows #191

Closed
wants to merge 8 commits into from
Closed
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 49 additions & 27 deletions Sources/TSCBasic/Path.swift
Original file line number Diff line number Diff line change
@@ -12,11 +12,7 @@ import Foundation
import WinSDK
#endif

#if os(Windows)
private typealias PathImpl = UNIXPath
#else
private typealias PathImpl = UNIXPath
#endif

/// Represents an absolute file system path, independently of what (or whether
/// anything at all) exists at that path in the file system at any given time.
@@ -438,12 +434,17 @@ private struct UNIXPath: Path {
static let root = UNIXPath(string: "/")

static func isValidComponent(_ name: String) -> Bool {
#if os(Windows)
if name.contains("\\") {
return false
}
#endif
return name != "" && name != "." && name != ".." && !name.contains("/")
}

#if os(Windows)
static func isAbsolutePath(_ path: String) -> Bool {
return !path.withCString(encodedAs: UTF16.self, PathIsRelativeW)
return !path.prenormalized().withCString(encodedAs: UTF16.self, PathIsRelativeW)
}
#endif

@@ -453,11 +454,12 @@ private struct UNIXPath: Path {
defer { fsr.deallocate() }

let path: String = String(cString: fsr)
return path.withCString(encodedAs: UTF16.self) {
let result: String = path.withCString(encodedAs: UTF16.self) {
let data = UnsafeMutablePointer(mutating: $0)
PathCchRemoveFileSpec(data, path.count)
return String(decodingCString: data, as: UTF16.self)
}
return result.isEmpty ? "." : result
#else
// FIXME: This method seems too complicated; it should be simplified,
// if possible, and certainly optimized (using UTF8View).
@@ -544,11 +546,13 @@ private struct UNIXPath: Path {

init(normalizingAbsolutePath path: String) {
#if os(Windows)
var buffer: [WCHAR] = Array<WCHAR>(repeating: 0, count: Int(MAX_PATH + 1))
_ = path.withCString(encodedAs: UTF16.self) {
PathCanonicalizeW(&buffer, $0)
var result: [WCHAR] = Array<WCHAR>(repeating: 0, count: Int(MAX_PATH + 1))
defer { LocalFree(result) }
stevapple marked this conversation as resolved.
Show resolved Hide resolved

_ = path.prenormalized().withCString(encodedAs: UTF16.self) {
PathCchCanonicalize($0, result.length, $0)
}
stevapple marked this conversation as resolved.
Show resolved Hide resolved
self.init(string: String(decodingCString: buffer, as: UTF16.self))
self.init(string: String(decodingCString: result, as: UTF16.self))
#else
compnerd marked this conversation as resolved.
Show resolved Hide resolved
precondition(path.first == "/", "Failure normalizing \(path), absolute paths should start with '/'")

@@ -613,14 +617,14 @@ private struct UNIXPath: Path {
}

init(normalizingRelativePath path: String) {
#if os(Windows)
var buffer: [WCHAR] = Array<WCHAR>(repeating: 0, count: Int(MAX_PATH + 1))
_ = path.replacingOccurrences(of: "/", with: "\\").withCString(encodedAs: UTF16.self) {
PathCanonicalizeW(&buffer, $0)
}
self.init(string: String(decodingCString: buffer, as: UTF16.self))
#else
precondition(path.first != "/")
let pathSeparator: Character
#if os(Windows)
pathSeparator = "\\"
let path = path.prenormalized()
#else
pathSeparator = "/"
#endif
precondition(path.first != pathSeparator)
Copy link
Member

Choose a reason for hiding this comment

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

This precondition doesn't hold. \path is valid as a relative path. It is relative to the current drive.

Copy link
Author

Choose a reason for hiding this comment

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

Although it is literally "relative", Windows Shell (and Swift which depends on it) regards \path \?\path \\share\path as absolute paths (according to the response of PathIsRelativeW). Personally I think these paths should be treated the same with a regular absolute path. For example, \path\..\.. indicates \ like C:\path\..\.. indicates C:\, but path\..\.. indicates ...

Copy link
Member

Choose a reason for hiding this comment

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

UNC paths are always absolute. It is just a drive relative path which is not absolute. That distinction is important as it impacts the lexical traversal of the path string.

Copy link
Author

Choose a reason for hiding this comment

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

PathIsRelativeW regards \path as an absolute path. Also, if we treat it as relative, behavior regarding .. will be unaligned.

Copy link
Author

Choose a reason for hiding this comment

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

Ping @compnerd

Copy link
Member

Choose a reason for hiding this comment

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

I still think that the differences are material and need to be addressed. Paths in Windows do not behave like Unix, and so you will have differences. e.g.

path1 = "C:\Users\compnerd"
path2 = "D:"
path1 + path2 is not `C:\Users\compnerd\D:" but rather "C:\Windows\System32\WindowsMetadata"

because the current directory on D: happened to be "....\Windows\System32\WindowsMetadata"

Copy link
Author

Choose a reason for hiding this comment

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

I’ll look into more critical cases here.

However, if I regard \path as a relative path, it will change the behavior of a bunch of APIs (while this PR focuses on bug fixes), so that change definitely needs to be addressed in a separate PR.

Copy link
Author

Choose a reason for hiding this comment

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

IMO I believe this PR is fully capable of fixing faulty behavior without changing the current judgement of relative paths and absolute paths. To fully support Windows path system, we’d better work on another one which may contain a complete refactoring.

What this PR mainly addressed and fixed is the problem of .. being ignored, and of crashing on an empty path, which are critical bugs for tools like SwiftPM.


// FIXME: Here we should also keep track of whether anything actually has
// to be changed in the string, and if not, just return the existing one.
@@ -630,7 +634,7 @@ private struct UNIXPath: Path {
// the normalized string representation.
var parts: [String] = []
var capacity = 0
for part in path.split(separator: "/") {
for part in path.split(separator: pathSeparator) {
switch part.count {
case 0:
// Ignore empty path components.
@@ -669,7 +673,7 @@ private struct UNIXPath: Path {
if let first = iter.next() {
result.append(contentsOf: first)
while let next = iter.next() {
result.append("/")
result.append(pathSeparator)
result.append(contentsOf: next)
}
}
@@ -680,11 +684,15 @@ private struct UNIXPath: Path {

// If the result is empty, return `.`, otherwise we return it as a string.
self.init(string: result.isEmpty ? "." : result)
compnerd marked this conversation as resolved.
Show resolved Hide resolved
#endif
}

init(validatingAbsolutePath path: String) throws {
#if os(Windows)
#if os(Windows)
// Explicitly handle the empty path, since retrieving
// `fileSystemRepresentation` of it is illegal.
guard !path.isEmpty else {
throw PathValidationError.invalidAbsolutePath(path)
}
let fsr: UnsafePointer<Int8> = path.fileSystemRepresentation
defer { fsr.deallocate() }

@@ -693,7 +701,7 @@ private struct UNIXPath: Path {
throw PathValidationError.invalidAbsolutePath(path)
}
self.init(normalizingAbsolutePath: path)
#else
#else
switch path.first {
case "/":
self.init(normalizingAbsolutePath: path)
@@ -702,11 +710,17 @@ private struct UNIXPath: Path {
default:
throw PathValidationError.invalidAbsolutePath(path)
}
#endif
#endif
}

init(validatingRelativePath path: String) throws {
#if os(Windows)
#if os(Windows)
// Explicitly handle the empty path, since retrieving
// `fileSystemRepresentation` of it is illegal.
guard !path.isEmpty else {
self.init(normalizingRelativePath: path)
return
}
compnerd marked this conversation as resolved.
Show resolved Hide resolved
let fsr: UnsafePointer<Int8> = path.fileSystemRepresentation
defer { fsr.deallocate() }

@@ -715,14 +729,14 @@ private struct UNIXPath: Path {
throw PathValidationError.invalidRelativePath(path)
}
self.init(normalizingRelativePath: path)
#else
#else
switch path.first {
case "/", "~":
throw PathValidationError.invalidRelativePath(path)
default:
self.init(normalizingRelativePath: path)
}
#endif
#endif
}

func suffix(withDot: Bool) -> String? {
@@ -941,3 +955,11 @@ private func mayNeedNormalization(absolute string: String) -> Bool {
}
return false
}

#if os(Windows)
fileprivate extension String {
func prenormalized() -> String {
stevapple marked this conversation as resolved.
Show resolved Hide resolved
return self.replacingOccurrences(of: "/", with: "\\")
}
}
#endif