From 0571e6e9b476c8f7ee4a8da8e40f07f9e0219d37 Mon Sep 17 00:00:00 2001 From: Alois Wohlschlager Date: Sat, 10 Feb 2024 20:56:54 +0100 Subject: [PATCH 1/2] Restore `builtins.pathExists` behavior on broken symlinks Commit 83c067c0fa0cc5a2dca440e5c986afe40b163802 changed `builtins.pathExists` to resolve symlinks before checking for existence. Consequently, if the path refers to a symlink itself, existence of the target of the symlink (instead of the symlink itself) was checked. Restore the previous behavior by skipping symlink resolution in the last component. (cherry picked from commit 89e21ab4bd1561c6eab2eeb63088f4e34fa4059f) --- src/libexpr/primops.cc | 15 +++++++----- src/libutil/source-path.cc | 22 ++++++++++------- src/libutil/source-path.hh | 24 +++++++++++++++---- .../functional/lang/eval-okay-pathexists.nix | 3 +++ .../functional/lang/symlink-resolution/broken | 1 + 5 files changed, 46 insertions(+), 19 deletions(-) create mode 120000 tests/functional/lang/symlink-resolution/broken diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index cdd9a3a098e..c10bee45968 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -118,7 +118,7 @@ StringMap EvalState::realiseContext(const NixStringContext & context) return res; } -static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v, bool resolveSymlinks = true) +static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v, std::optional resolveSymlinks = SymlinkResolution::Full) { NixStringContext context; @@ -130,7 +130,7 @@ static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v, bo auto realPath = state.toRealPath(rewriteStrings(path.path.abs(), rewrites), context); path = {path.accessor, CanonPath(realPath)}; } - return resolveSymlinks ? path.resolveSymlinks() : path; + return resolveSymlinks ? path.resolveSymlinks(*resolveSymlinks) : path; } catch (Error & e) { e.addTrace(state.positions[pos], "while realising the context of path '%s'", path); throw; @@ -170,7 +170,7 @@ static void mkOutputString( argument. */ static void import(EvalState & state, const PosIdx pos, Value & vPath, Value * vScope, Value & v) { - auto path = realisePath(state, pos, vPath, false); + auto path = realisePath(state, pos, vPath, std::nullopt); auto path2 = path.path.abs(); // FIXME @@ -1534,13 +1534,16 @@ static void prim_pathExists(EvalState & state, const PosIdx pos, Value * * args, try { auto & arg = *args[0]; - auto path = realisePath(state, pos, arg); - /* SourcePath doesn't know about trailing slash. */ + state.forceValue(arg, pos); auto mustBeDir = arg.type() == nString && (arg.string_view().ends_with("/") || arg.string_view().ends_with("/.")); + auto symlinkResolution = + mustBeDir ? SymlinkResolution::Full : SymlinkResolution::Ancestors; + auto path = realisePath(state, pos, arg, symlinkResolution); + auto st = path.maybeLstat(); auto exists = st && (!mustBeDir || st->type == SourceAccessor::tDirectory); v.mkBool(exists); @@ -1777,7 +1780,7 @@ static std::string_view fileTypeToString(InputAccessor::Type type) static void prim_readFileType(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - auto path = realisePath(state, pos, *args[0], false); + auto path = realisePath(state, pos, *args[0], std::nullopt); /* Retrieve the directory entry type and stringize it. */ v.mkString(fileTypeToString(path.lstat().type)); } diff --git a/src/libutil/source-path.cc b/src/libutil/source-path.cc index d85b0b7fe8a..188aaf61621 100644 --- a/src/libutil/source-path.cc +++ b/src/libutil/source-path.cc @@ -62,7 +62,7 @@ bool SourcePath::operator<(const SourcePath & x) const return std::tie(*accessor, path) < std::tie(*x.accessor, x.path); } -SourcePath SourcePath::resolveSymlinks() const +SourcePath SourcePath::resolveSymlinks(SymlinkResolution mode) const { auto res = SourcePath(accessor); @@ -72,6 +72,8 @@ SourcePath SourcePath::resolveSymlinks() const for (auto & c : path) todo.push_back(std::string(c)); + bool resolve_last = mode == SymlinkResolution::Full; + while (!todo.empty()) { auto c = *todo.begin(); todo.pop_front(); @@ -81,14 +83,16 @@ SourcePath SourcePath::resolveSymlinks() const res.path.pop(); else { res.path.push(c); - if (auto st = res.maybeLstat(); st && st->type == InputAccessor::tSymlink) { - if (!linksAllowed--) - throw Error("infinite symlink recursion in path '%s'", path); - auto target = res.readLink(); - res.path.pop(); - if (hasPrefix(target, "/")) - res.path = CanonPath::root; - todo.splice(todo.begin(), tokenizeString>(target, "/")); + if (resolve_last || !todo.empty()) { + if (auto st = res.maybeLstat(); st && st->type == InputAccessor::tSymlink) { + if (!linksAllowed--) + throw Error("infinite symlink recursion in path '%s'", path); + auto target = res.readLink(); + res.path.pop(); + if (hasPrefix(target, "/")) + res.path = CanonPath::root; + todo.splice(todo.begin(), tokenizeString>(target, "/")); + } } } } diff --git a/src/libutil/source-path.hh b/src/libutil/source-path.hh index bf5625ca583..c48490065f9 100644 --- a/src/libutil/source-path.hh +++ b/src/libutil/source-path.hh @@ -11,6 +11,22 @@ namespace nix { +enum class SymlinkResolution { + /** + * Resolve symlinks in the ancestors only. + * + * Only the last component of the result is possibly a symlink. + */ + Ancestors, + + /** + * Resolve symlinks fully, realpath(3)-style. + * + * No component of the result will be a symlink. + */ + Full, +}; + /** * An abstraction for accessing source files during * evaluation. Currently, it's just a wrapper around `CanonPath` that @@ -102,11 +118,11 @@ struct SourcePath bool operator<(const SourcePath & x) const; /** - * Resolve any symlinks in this `SourcePath` (including its - * parents). The result is a `SourcePath` in which no element is a - * symlink. + * Resolve any symlinks in this `SourcePath` according to the + * given resolution mode. */ - SourcePath resolveSymlinks() const; + SourcePath resolveSymlinks( + SymlinkResolution mode = SymlinkResolution::Full) const; }; std::ostream & operator << (std::ostream & str, const SourcePath & path); diff --git a/tests/functional/lang/eval-okay-pathexists.nix b/tests/functional/lang/eval-okay-pathexists.nix index 31697f66a90..022b22feae5 100644 --- a/tests/functional/lang/eval-okay-pathexists.nix +++ b/tests/functional/lang/eval-okay-pathexists.nix @@ -29,3 +29,6 @@ builtins.pathExists (./lib.nix) && builtins.pathExists (builtins.toPath { outPath = builtins.toString ./lib.nix; }) && builtins.pathExists ./lib.nix && !builtins.pathExists ./bla.nix +&& builtins.pathExists ./symlink-resolution/foo/overlays/overlay.nix +&& builtins.pathExists ./symlink-resolution/broken +&& builtins.pathExists (builtins.toString ./symlink-resolution/foo/overlays + "/.") diff --git a/tests/functional/lang/symlink-resolution/broken b/tests/functional/lang/symlink-resolution/broken new file mode 120000 index 00000000000..e07da690b56 --- /dev/null +++ b/tests/functional/lang/symlink-resolution/broken @@ -0,0 +1 @@ +nonexistent \ No newline at end of file From 02f7025debfdf00b53f025cab92a0a8c3c85bf9e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 16 Feb 2024 08:45:15 -0500 Subject: [PATCH 2/2] Add note about this being a temp solution (cherry picked from commit e27b7e04bf38c1fdf342d6e15b2c003ca9b92cb1) --- src/libutil/source-path.hh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/libutil/source-path.hh b/src/libutil/source-path.hh index c48490065f9..9544a08d85f 100644 --- a/src/libutil/source-path.hh +++ b/src/libutil/source-path.hh @@ -11,6 +11,10 @@ namespace nix { +/** + * Note there is a decent chance this type soon goes away because the problem is solved another way. + * See the discussion in https://github.com/NixOS/nix/pull/9985. + */ enum class SymlinkResolution { /** * Resolve symlinks in the ancestors only. @@ -120,6 +124,9 @@ struct SourcePath /** * Resolve any symlinks in this `SourcePath` according to the * given resolution mode. + * + * @param mode might only be a temporary solution for this. + * See the discussion in https://github.com/NixOS/nix/pull/9985. */ SourcePath resolveSymlinks( SymlinkResolution mode = SymlinkResolution::Full) const;