Commit `3ae4931dc1`

Andrew Kelley <andrew@ziglang.org>

2022-11-26 01:12:50

CLI: more careful resolution of paths

In general, we prefer compiler code to use relative paths based on open directory handles because this is the most portable. However, sometimes absolute paths are used, and sometimes relative paths are used that go up a directory. The recent improvements in 81d2135ca6ebd71b8c121a19957c8fbf7f87125b regressed the use case when an absolute path is used for the zig lib directory mixed with a relative path used for the root source file. This could happen when, for example, running the standard library tests, like this: stage3/bin/zig test ../lib/std/std.zig This happened because the zig lib dir was inferred to be an absolute directory based on the zig executable directory, while the root source file was detected as a relative path. There was no common prefix and so it was not determined that the std.zig file was inside the lib directory. This commit adds a function for resolving paths that preserves relative path names while allowing absolute paths, and converting relative upwards paths (e.g. "../foo") to absolute paths. This restores the previous functionality while remaining compatible with systems such as WASI that cannot deal with absolute paths.

master

1 parent 7411be3

Changed files (6)

lib

std

src

@@ -462,7 +462,6 @@ pub fn resolve(allocator: Allocator, paths: []const []const u8) ![]u8 {
 /// This function is like a series of `cd` statements executed one after another.
 /// It resolves "." and "..".
 /// The result does not have a trailing path separator.
-/// If all paths are relative it uses the current working directory as a starting point.
 /// Each drive has its own current working directory.
 /// Path separators are canonicalized to '\\' and drives are canonicalized to capital letters.
 /// Note: all usage of this function should be audited due to the existence of symlinks.
@@ -572,15 +571,15 @@ pub fn resolveWindows(allocator: Allocator, paths: []const []const u8) ![]u8 {
             continue;
         }
         var it = mem.tokenize(u8, p[parsed.disk_designator.len..], "/\\");
-        component: while (it.next()) |component| {
+        while (it.next()) |component| {
             if (mem.eql(u8, component, ".")) {
                 continue;
             } else if (mem.eql(u8, component, "..")) {
+                if (result.items.len == 0) {
+                    negative_count += 1;
+                    continue;
+                }
                 while (true) {
-                    if (result.items.len == 0) {
-                        negative_count += 1;
-                        continue :component;
-                    }
                     if (result.items.len == disk_designator_len) {
                         break;
                     }
@@ -589,7 +588,7 @@ pub fn resolveWindows(allocator: Allocator, paths: []const []const u8) ![]u8 {
                         else => false,
                     };
                     result.items.len -= 1;
-                    if (end_with_sep) break;
+                    if (end_with_sep or result.items.len == 0) break;
                 }
             } else if (!have_abs_path and result.items.len == 0) {
                 try result.appendSlice(component);
@@ -659,18 +658,18 @@ pub fn resolvePosix(allocator: Allocator, paths: []const []const u8) Allocator.E
             result.clearRetainingCapacity();
         }
         var it = mem.tokenize(u8, p, "/");
-        component: while (it.next()) |component| {
+        while (it.next()) |component| {
             if (mem.eql(u8, component, ".")) {
                 continue;
             } else if (mem.eql(u8, component, "..")) {
+                if (result.items.len == 0) {
+                    negative_count += @boolToInt(!is_abs);
+                    continue;
+                }
                 while (true) {
-                    if (result.items.len == 0) {
-                        negative_count += @boolToInt(!is_abs);
-                        continue :component;
-                    }
                     const ends_with_slash = result.items[result.items.len - 1] == '/';
                     result.items.len -= 1;
-                    if (ends_with_slash) break;
+                    if (ends_with_slash or result.items.len == 0) break;
                 }
             } else if (result.items.len > 0 or is_abs) {
                 try result.ensureUnusedCapacity(1 + component.len);
@@ -717,10 +716,10 @@ pub fn resolvePosix(allocator: Allocator, paths: []const []const u8) Allocator.E
 }
 
 test "resolve" {
-    try testResolveWindows(&[_][]const u8{ "a\\b\\c\\", "..\\..\\.." }, "..");
+    try testResolveWindows(&[_][]const u8{ "a\\b\\c\\", "..\\..\\.." }, ".");
     try testResolveWindows(&[_][]const u8{"."}, ".");
 
-    try testResolvePosix(&[_][]const u8{ "a/b/c/", "../../.." }, "..");
+    try testResolvePosix(&[_][]const u8{ "a/b/c/", "../../.." }, ".");
     try testResolvePosix(&[_][]const u8{"."}, ".");
 }
 
@@ -753,19 +752,21 @@ test "resolveWindows" {
 }
 
 test "resolvePosix" {
-    try testResolvePosix(&[_][]const u8{ "/a/b", "c" }, "/a/b/c");
-    try testResolvePosix(&[_][]const u8{ "/a/b", "c", "//d", "e///" }, "/d/e");
-    try testResolvePosix(&[_][]const u8{ "/a/b/c", "..", "../" }, "/a");
-    try testResolvePosix(&[_][]const u8{ "/", "..", ".." }, "/");
-    try testResolvePosix(&[_][]const u8{"/a/b/c/"}, "/a/b/c");
+    try testResolvePosix(&.{ "/a/b", "c" }, "/a/b/c");
+    try testResolvePosix(&.{ "/a/b", "c", "//d", "e///" }, "/d/e");
+    try testResolvePosix(&.{ "/a/b/c", "..", "../" }, "/a");
+    try testResolvePosix(&.{ "/", "..", ".." }, "/");
+    try testResolvePosix(&.{"/a/b/c/"}, "/a/b/c");
 
-    try testResolvePosix(&[_][]const u8{ "/var/lib", "../", "file/" }, "/var/file");
-    try testResolvePosix(&[_][]const u8{ "/var/lib", "/../", "file/" }, "/file");
-    try testResolvePosix(&[_][]const u8{ "/some/dir", ".", "/absolute/" }, "/absolute");
-    try testResolvePosix(&[_][]const u8{ "/foo/tmp.3/", "../tmp.3/cycles/root.js" }, "/foo/tmp.3/cycles/root.js");
+    try testResolvePosix(&.{ "/var/lib", "../", "file/" }, "/var/file");
+    try testResolvePosix(&.{ "/var/lib", "/../", "file/" }, "/file");
+    try testResolvePosix(&.{ "/some/dir", ".", "/absolute/" }, "/absolute");
+    try testResolvePosix(&.{ "/foo/tmp.3/", "../tmp.3/cycles/root.js" }, "/foo/tmp.3/cycles/root.js");
 
     // Keep relative paths relative.
-    try testResolvePosix(&[_][]const u8{"a/b"}, "a/b");
+    try testResolvePosix(&.{"a/b"}, "a/b");
+    try testResolvePosix(&.{"."}, ".");
+    try testResolvePosix(&.{ ".", "src/test.zig", "..", "../test/cases.zig" }, "test/cases.zig");
 }
 
 fn testResolveWindows(paths: []const []const u8, expected: []const u8) !void {

@@ -82,7 +82,12 @@ pub fn findZigLibDir(gpa: mem.Allocator) !Compilation.Directory {
 pub fn findZigLibDirFromSelfExe(
     allocator: mem.Allocator,
     self_exe_path: []const u8,
-) error{ OutOfMemory, FileNotFound }!Compilation.Directory {
+) error{
+    OutOfMemory,
+    FileNotFound,
+    CurrentWorkingDirectoryUnlinked,
+    Unexpected,
+}!Compilation.Directory {
     const cwd = fs.cwd();
     var cur_path: []const u8 = self_exe_path;
     while (fs.path.dirname(cur_path)) |dirname| : (cur_path = dirname) {
@@ -90,9 +95,11 @@ pub fn findZigLibDirFromSelfExe(
         defer base_dir.close();
 
         const sub_directory = testZigInstallPrefix(base_dir) orelse continue;
+        const p = try fs.path.join(allocator, &[_][]const u8{ dirname, sub_directory.path.? });
+        defer allocator.free(p);
         return Compilation.Directory{
             .handle = sub_directory.handle,
-            .path = try fs.path.join(allocator, &[_][]const u8{ dirname, sub_directory.path.? }),
+            .path = try resolvePath(allocator, p),
         };
     }
     return error.FileNotFound;
@@ -130,3 +137,46 @@ pub fn resolveGlobalCacheDir(allocator: mem.Allocator) ![]u8 {
         return fs.getAppDataDir(allocator, appname);
     }
 }
+
+/// Similar to std.fs.path.resolve, with a few important differences:
+/// * If the input is an absolute path, check it against the cwd and try to
+///   convert it to a relative path.
+/// * If the resulting path would start with a relative up-dir ("../"), instead
+///   return an absolute path based on the cwd.
+/// * When targeting WASI, fail with an error message if an absolute path is
+///   used.
+pub fn resolvePath(
+    ally: mem.Allocator,
+    p: []const u8,
+) error{
+    OutOfMemory,
+    CurrentWorkingDirectoryUnlinked,
+    Unexpected,
+}![]u8 {
+    if (fs.path.isAbsolute(p)) {
+        const cwd_path = try std.process.getCwdAlloc(ally);
+        defer ally.free(cwd_path);
+        const relative = try fs.path.relative(ally, cwd_path, p);
+        if (isUpDir(relative)) {
+            ally.free(relative);
+            return ally.dupe(u8, p);
+        } else {
+            return relative;
+        }
+    } else {
+        const resolved = try fs.path.resolve(ally, &.{p});
+        if (isUpDir(resolved)) {
+            ally.free(resolved);
+            const cwd_path = try std.process.getCwdAlloc(ally);
+            defer ally.free(cwd_path);
+            return fs.path.resolve(ally, &.{ cwd_path, p });
+        } else {
+            return resolved;
+        }
+    }
+}
+
+/// TODO move this to std.fs.path
+pub fn isUpDir(p: []const u8) bool {
+    return mem.startsWith(u8, p, "..") and (p.len == 2 or p[2] == fs.path.sep);
+}

@@ -885,24 +885,28 @@ fn buildOutputType(
                             fatal("unexpected end-of-parameter mark: --", .{});
                         }
                     } else if (mem.eql(u8, arg, "--pkg-begin")) {
-                        const pkg_name = args_iter.next();
-                        const pkg_path = args_iter.next();
-                        if (pkg_name == null or pkg_path == null) fatal("Expected 2 arguments after {s}", .{arg});
+                        const opt_pkg_name = args_iter.next();
+                        const opt_pkg_path = args_iter.next();
+                        if (opt_pkg_name == null or opt_pkg_path == null)
+                            fatal("Expected 2 arguments after {s}", .{arg});
+
+                        const pkg_name = opt_pkg_name.?;
+                        const pkg_path = try introspect.resolvePath(arena, opt_pkg_path.?);
 
                         const new_cur_pkg = Package.create(
                             gpa,
-                            fs.path.dirname(pkg_path.?),
-                            fs.path.basename(pkg_path.?),
+                            fs.path.dirname(pkg_path),
+                            fs.path.basename(pkg_path),
                         ) catch |err| {
-                            fatal("Failed to add package at path {s}: {s}", .{ pkg_path.?, @errorName(err) });
+                            fatal("Failed to add package at path {s}: {s}", .{ pkg_path, @errorName(err) });
                         };
 
-                        if (mem.eql(u8, pkg_name.?, "std") or mem.eql(u8, pkg_name.?, "root") or mem.eql(u8, pkg_name.?, "builtin")) {
-                            fatal("unable to add package '{s}' -> '{s}': conflicts with builtin package", .{ pkg_name.?, pkg_path.? });
-                        } else if (cur_pkg.table.get(pkg_name.?)) |prev| {
-                            fatal("unable to add package '{s}' -> '{s}': already exists as '{s}", .{ pkg_name.?, pkg_path.?, prev.root_src_path });
+                        if (mem.eql(u8, pkg_name, "std") or mem.eql(u8, pkg_name, "root") or mem.eql(u8, pkg_name, "builtin")) {
+                            fatal("unable to add package '{s}' -> '{s}': conflicts with builtin package", .{ pkg_name, pkg_path });
+                        } else if (cur_pkg.table.get(pkg_name)) |prev| {
+                            fatal("unable to add package '{s}' -> '{s}': already exists as '{s}", .{ pkg_name, pkg_path, prev.root_src_path });
                         }
-                        try cur_pkg.addAndAdopt(gpa, pkg_name.?, new_cur_pkg);
+                        try cur_pkg.addAndAdopt(gpa, pkg_name, new_cur_pkg);
                         cur_pkg = new_cur_pkg;
                     } else if (mem.eql(u8, arg, "--pkg-end")) {
                         cur_pkg = cur_pkg.parent orelse
@@ -2705,11 +2709,16 @@ fn buildOutputType(
     };
     defer emit_implib_resolved.deinit();
 
-    const main_pkg: ?*Package = if (root_src_file) |src_path| blk: {
-        if (main_pkg_path) |p| {
-            const rel_src_path = try fs.path.relative(gpa, p, src_path);
-            defer gpa.free(rel_src_path);
-            break :blk try Package.create(gpa, p, rel_src_path);
+    const main_pkg: ?*Package = if (root_src_file) |unresolved_src_path| blk: {
+        const src_path = try introspect.resolvePath(arena, unresolved_src_path);
+        if (main_pkg_path) |unresolved_main_pkg_path| {
+            const p = try introspect.resolvePath(arena, unresolved_main_pkg_path);
+            if (p.len == 0) {
+                break :blk try Package.create(gpa, null, src_path);
+            } else {
+                const rel_src_path = try fs.path.relative(arena, p, src_path);
+                break :blk try Package.create(gpa, p, rel_src_path);
+            }
         } else {
             const root_src_dir_path = fs.path.dirname(src_path);
             break :blk Package.create(gpa, root_src_dir_path, fs.path.basename(src_path)) catch |err| {
@@ -2745,7 +2754,7 @@ fn buildOutputType(
 
     const self_exe_path = try introspect.findZigExePath(arena);
     var zig_lib_directory: Compilation.Directory = if (override_lib_dir) |unresolved_lib_dir| l: {
-        const lib_dir = try fs.path.resolve(arena, &.{unresolved_lib_dir});
+        const lib_dir = try introspect.resolvePath(arena, unresolved_lib_dir);
         break :l .{
             .path = lib_dir,
             .handle = fs.cwd().openDir(lib_dir, .{}) catch |err| {

@@ -30,6 +30,7 @@ const Sema = @import("Sema.zig");
 const target_util = @import("target.zig");
 const build_options = @import("build_options");
 const Liveness = @import("Liveness.zig");
+const isUpDir = @import("introspect.zig").isUpDir;
 
 /// General-purpose allocator. Used for both temporary and long-term storage.
 gpa: Allocator,
@@ -4957,15 +4958,19 @@ pub fn importFile(
     const resolved_root_path = try std.fs.path.resolve(gpa, &[_][]const u8{cur_pkg_dir_path});
     defer gpa.free(resolved_root_path);
 
-    if (!mem.startsWith(u8, resolved_path, resolved_root_path) or
-        // This prevents this check from triggering when the name of the
-        // imported file starts with the root path's directory name.
-        !std.fs.path.isSep(resolved_path[resolved_root_path.len]))
-    {
+    const sub_file_path = p: {
+        if (mem.startsWith(u8, resolved_path, resolved_root_path)) {
+            // +1 for the directory separator here.
+            break :p try gpa.dupe(u8, resolved_path[resolved_root_path.len + 1 ..]);
+        }
+        if (mem.eql(u8, resolved_root_path, ".") and
+            !isUpDir(resolved_path) and
+            !std.fs.path.isAbsolute(resolved_path))
+        {
+            break :p try gpa.dupe(u8, resolved_path);
+        }
         return error.ImportOutsidePkgPath;
-    }
-    // +1 for the directory separator here.
-    const sub_file_path = try gpa.dupe(u8, resolved_path[resolved_root_path.len + 1 ..]);
+    };
     errdefer gpa.free(sub_file_path);
 
     log.debug("new importFile. resolved_root_path={s}, resolved_path={s}, sub_file_path={s}, import_string={s}", .{
@@ -5015,11 +5020,19 @@ pub fn embedFile(mod: *Module, cur_file: *File, rel_file_path: []const u8) !*Emb
     const resolved_root_path = try std.fs.path.resolve(gpa, &[_][]const u8{cur_pkg_dir_path});
     defer gpa.free(resolved_root_path);
 
-    if (!mem.startsWith(u8, resolved_path, resolved_root_path)) {
+    const sub_file_path = p: {
+        if (mem.startsWith(u8, resolved_path, resolved_root_path)) {
+            // +1 for the directory separator here.
+            break :p try gpa.dupe(u8, resolved_path[resolved_root_path.len + 1 ..]);
+        }
+        if (mem.eql(u8, resolved_root_path, ".") and
+            !isUpDir(resolved_path) and
+            !std.fs.path.isAbsolute(resolved_path))
+        {
+            break :p try gpa.dupe(u8, resolved_path);
+        }
         return error.ImportOutsidePkgPath;
-    }
-    // +1 for the directory separator here.
-    const sub_file_path = try gpa.dupe(u8, resolved_path[resolved_root_path.len + 1 ..]);
+    };
     errdefer gpa.free(sub_file_path);
 
     var file = try cur_file.pkg.root_src_directory.handle.openFile(sub_file_path, .{});

@@ -60,7 +60,7 @@ test {
         ctx.addTestCasesFromDir(dir);
     }
 
-    try @import("test_cases").addCases(&ctx);
+    try @import("../test/cases.zig").addCases(&ctx);
 
     try ctx.run();
 }

@@ -41,9 +41,9 @@ pub fn build(b: *Builder) !void {
     docs_step.dependOn(&docgen_cmd.step);
 
     const test_cases = b.addTest("src/test.zig");
+    test_cases.main_pkg_path = ".";
     test_cases.stack_size = stack_size;
     test_cases.setBuildMode(mode);
-    test_cases.addPackagePath("test_cases", "test/cases.zig");
     test_cases.single_threaded = single_threaded;
 
     const fmt_build_zig = b.addFmt(&[_][]const u8{"build.zig"});

Commit 3ae4931dc1

Commit `3ae4931dc1`