Skip to content

Fix symLink's handling of / path separators on Windows #19136

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

Merged
merged 3 commits into from
Mar 2, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
10 changes: 10 additions & 0 deletions lib/std/fs/Dir.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1705,6 +1705,15 @@ pub fn symLink(
var target_path_w: std.os.windows.PathSpace = undefined;
target_path_w.len = try std.unicode.wtf8ToWtf16Le(&target_path_w.data, target_path);
target_path_w.data[target_path_w.len] = 0;
// However, we need to canonicalize any path separators to `\`, since if
// the target path is relative, then it must use `\` as the path separator.
mem.replaceScalar(
u16,
target_path_w.data[0..target_path_w.len],
mem.nativeToLittle(u16, '/'),
mem.nativeToLittle(u16, '\\'),
);

const sym_link_path_w = try std.os.windows.sliceToPrefixedFileW(self.fd, sym_link_path);
return self.symLinkW(target_path_w.span(), sym_link_path_w.span(), flags);
}
Expand Down Expand Up @@ -1744,6 +1753,7 @@ pub fn symLinkW(
self: Dir,
/// WTF-16, does not need to be NT-prefixed. The NT-prefixing
/// of this path is handled by CreateSymbolicLink.
/// Any path separators must be `\`, not `/`.
target_path_w: [:0]const u16,
/// WTF-16, must be NT-prefixed or relative
sym_link_path_w: []const u16,
Expand Down
61 changes: 49 additions & 12 deletions lib/std/fs/test.zig
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,17 @@ const PathType = enum {

const TestContext = struct {
path_type: PathType,
path_sep: u8,
arena: ArenaAllocator,
tmp: testing.TmpDir,
dir: std.fs.Dir,
transform_fn: *const PathType.TransformFn,

pub fn init(path_type: PathType, allocator: mem.Allocator, transform_fn: *const PathType.TransformFn) TestContext {
pub fn init(path_type: PathType, path_sep: u8, allocator: mem.Allocator, transform_fn: *const PathType.TransformFn) TestContext {
const tmp = tmpDir(.{ .iterate = true });
return .{
.path_type = path_type,
.path_sep = path_sep,
.arena = ArenaAllocator.init(allocator),
.tmp = tmp,
.dir = tmp.dir,
Expand All @@ -93,11 +95,37 @@ const TestContext = struct {
self.tmp.cleanup();
}

/// Returns the `relative_path` transformed into the TestContext's `path_type`.
/// Returns the `relative_path` transformed into the TestContext's `path_type`,
/// with any supported path separators replaced by `path_sep`.
/// The result is allocated by the TestContext's arena and will be free'd during
/// `TestContext.deinit`.
pub fn transformPath(self: *TestContext, relative_path: [:0]const u8) ![:0]const u8 {
return self.transform_fn(self.arena.allocator(), self.dir, relative_path);
const allocator = self.arena.allocator();
const transformed_path = try self.transform_fn(allocator, self.dir, relative_path);
if (builtin.os.tag == .windows) {
const transformed_sep_path = try allocator.dupeZ(u8, transformed_path);
std.mem.replaceScalar(u8, transformed_sep_path, switch (self.path_sep) {
'/' => '\\',
'\\' => '/',
else => unreachable,
}, self.path_sep);
return transformed_sep_path;
}
return transformed_path;
}

/// Replaces any path separators with the canonical path separator for the platform
/// (e.g. all path separators are converted to `\` on Windows).
/// If path separators are replaced, then the result is allocated by the
/// TestContext's arena and will be free'd during `TestContext.deinit`.
pub fn toCanonicalPathSep(self: *TestContext, path: [:0]const u8) ![:0]const u8 {
if (builtin.os.tag == .windows) {
const allocator = self.arena.allocator();
const transformed_sep_path = try allocator.dupeZ(u8, path);
std.mem.replaceScalar(u8, transformed_sep_path, '/', '\\');
return transformed_sep_path;
}
return path;
}
};

Expand All @@ -106,15 +134,19 @@ const TestContext = struct {
/// and will be passed a TestContext that can transform a relative path into the path type under test.
/// The TestContext will also create a tmp directory for you (and will clean it up for you too).
fn testWithAllSupportedPathTypes(test_func: anytype) !void {
try testWithPathTypeIfSupported(.relative, test_func);
try testWithPathTypeIfSupported(.absolute, test_func);
try testWithPathTypeIfSupported(.unc, test_func);
try testWithPathTypeIfSupported(.relative, '/', test_func);
try testWithPathTypeIfSupported(.absolute, '/', test_func);
try testWithPathTypeIfSupported(.unc, '/', test_func);
try testWithPathTypeIfSupported(.relative, '\\', test_func);
try testWithPathTypeIfSupported(.absolute, '\\', test_func);
try testWithPathTypeIfSupported(.unc, '\\', test_func);
}

fn testWithPathTypeIfSupported(comptime path_type: PathType, test_func: anytype) !void {
fn testWithPathTypeIfSupported(comptime path_type: PathType, comptime path_sep: u8, test_func: anytype) !void {
if (!(comptime path_type.isSupported(builtin.os))) return;
if (!(comptime fs.path.isSep(path_sep))) return;

var ctx = TestContext.init(path_type, testing.allocator, path_type.getTransformFn());
var ctx = TestContext.init(path_type, path_sep, testing.allocator, path_type.getTransformFn());
defer ctx.deinit();

try test_func(&ctx);
Expand Down Expand Up @@ -148,20 +180,25 @@ test "Dir.readLink" {
const dir_target_path = try ctx.transformPath("subdir");
try ctx.dir.makeDir(dir_target_path);

// On Windows, symlink targets always use the canonical path separator
const canonical_file_target_path = try ctx.toCanonicalPathSep(file_target_path);
const canonical_dir_target_path = try ctx.toCanonicalPathSep(dir_target_path);

// test 1: symlink to a file
try setupSymlink(ctx.dir, file_target_path, "symlink1", .{});
try testReadLink(ctx.dir, file_target_path, "symlink1");
try testReadLink(ctx.dir, canonical_file_target_path, "symlink1");

// test 2: symlink to a directory (can be different on Windows)
try setupSymlink(ctx.dir, dir_target_path, "symlink2", .{ .is_directory = true });
try testReadLink(ctx.dir, dir_target_path, "symlink2");
try testReadLink(ctx.dir, canonical_dir_target_path, "symlink2");

// test 3: relative path symlink
const parent_file = ".." ++ fs.path.sep_str ++ "target.txt";
const canonical_parent_file = try ctx.toCanonicalPathSep(parent_file);
var subdir = try ctx.dir.makeOpenPath("subdir", .{});
defer subdir.close();
try setupSymlink(subdir, parent_file, "relative-link.txt", .{});
try testReadLink(subdir, parent_file, "relative-link.txt");
try setupSymlink(subdir, canonical_parent_file, "relative-link.txt", .{});
try testReadLink(subdir, canonical_parent_file, "relative-link.txt");
}
}.impl);
}
Expand Down
5 changes: 1 addition & 4 deletions lib/std/tar/test.zig
Original file line number Diff line number Diff line change
Expand Up @@ -464,9 +464,6 @@ test "tar case sensitivity" {
}

test "tar pipeToFileSystem" {
const builtin = @import("builtin");
if (builtin.os.tag == .windows) return error.SkipZigTest;

// $ tar tvf
// pipe_to_file_system_test/
// pipe_to_file_system_test/b/
Expand Down Expand Up @@ -494,6 +491,6 @@ test "tar pipeToFileSystem" {
try testing.expect((try root.dir.statFile("a/file")).kind == .file);
// TODO is there better way to test symlink
try testing.expect((try root.dir.statFile("b/symlink")).kind == .file); // statFile follows symlink
var buf: [8]u8 = undefined;
var buf: [32]u8 = undefined;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

_ = try root.dir.readLink("b/symlink", &buf);
}