From 85b9f4ef4fcc7b7fc03ff9503f280d736b5c65c7 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 10 Apr 2024 23:46:19 +0200 Subject: [PATCH 1/3] nix shell: Handle output paths that are symlinks This requires moving resolveSymlinks() into SourceAccessor. Also, it requires LocalStoreAccessor::maybeLstat() to work on parents of the store (to avoid an error like "/nix is not in the store"). Fixes #10375. --- src/libstore/local-fs-store.cc | 4 +++ src/libutil/source-accessor.cc | 44 +++++++++++++++++++++++++++++--- src/libutil/source-accessor.hh | 37 ++++++++++++++++++++++++--- src/libutil/source-path.cc | 38 --------------------------- src/libutil/source-path.hh | 31 ++++------------------ src/nix/unix/run.cc | 3 ++- tests/functional/shell-hello.nix | 10 +++++++- tests/functional/shell.sh | 2 ++ 8 files changed, 97 insertions(+), 72 deletions(-) diff --git a/src/libstore/local-fs-store.cc b/src/libstore/local-fs-store.cc index 81c385ddb9e..843c0d28881 100644 --- a/src/libstore/local-fs-store.cc +++ b/src/libstore/local-fs-store.cc @@ -33,6 +33,10 @@ struct LocalStoreAccessor : PosixSourceAccessor std::optional maybeLstat(const CanonPath & path) override { + /* Handle the case where `path` is (a parent of) the store. */ + if (isDirOrInDir(store->storeDir, path.abs())) + return Stat{ .type = tDirectory }; + return PosixSourceAccessor::maybeLstat(toRealPath(path)); } diff --git a/src/libutil/source-accessor.cc b/src/libutil/source-accessor.cc index afbbbe1a9c5..66093d2ccb0 100644 --- a/src/libutil/source-accessor.cc +++ b/src/libutil/source-accessor.cc @@ -39,9 +39,9 @@ void SourceAccessor::readFile( } Hash SourceAccessor::hashPath( - const CanonPath & path, - PathFilter & filter, - HashAlgorithm ha) + const CanonPath & path, + PathFilter & filter, + HashAlgorithm ha) { HashSink sink(ha); dumpPath(path, sink, filter); @@ -67,4 +67,42 @@ std::string SourceAccessor::showPath(const CanonPath & path) return displayPrefix + path.abs() + displaySuffix; } +CanonPath SourceAccessor::resolveSymlinks( + const CanonPath & path, + SymlinkResolution mode) +{ + auto res = CanonPath::root; + + int linksAllowed = 1024; + + std::list todo; + for (auto & c : path) + todo.push_back(std::string(c)); + + while (!todo.empty()) { + auto c = *todo.begin(); + todo.pop_front(); + if (c == "" || c == ".") + ; + else if (c == "..") + res.pop(); + else { + res.push(c); + if (mode == SymlinkResolution::Full || !todo.empty()) { + if (auto st = maybeLstat(res); st && st->type == SourceAccessor::tSymlink) { + if (!linksAllowed--) + throw Error("infinite symlink recursion in path '%s'", showPath(path)); + auto target = readLink(res); + res.pop(); + if (hasPrefix(target, "/")) + res = CanonPath::root; + todo.splice(todo.begin(), tokenizeString>(target, "/")); + } + } + } + } + + return res; +} + } diff --git a/src/libutil/source-accessor.hh b/src/libutil/source-accessor.hh index aff7da09c44..1f272327f81 100644 --- a/src/libutil/source-accessor.hh +++ b/src/libutil/source-accessor.hh @@ -9,6 +9,26 @@ namespace nix { struct Sink; +/** + * 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. + * + * 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, +}; + /** * A read-only filesystem abstraction. This is used by the Nix * evaluator and elsewhere for accessing sources in various @@ -112,9 +132,9 @@ struct SourceAccessor PathFilter & filter = defaultPathFilter); Hash hashPath( - const CanonPath & path, - PathFilter & filter = defaultPathFilter, - HashAlgorithm ha = HashAlgorithm::SHA256); + const CanonPath & path, + PathFilter & filter = defaultPathFilter, + HashAlgorithm ha = HashAlgorithm::SHA256); /** * Return a corresponding path in the root filesystem, if @@ -137,6 +157,17 @@ struct SourceAccessor void setPathDisplay(std::string displayPrefix, std::string displaySuffix = ""); virtual std::string showPath(const CanonPath & path); + + /** + * Resolve any symlinks in `path` 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. + */ + CanonPath resolveSymlinks( + const CanonPath & path, + SymlinkResolution mode = SymlinkResolution::Full); }; } diff --git a/src/libutil/source-path.cc b/src/libutil/source-path.cc index 56ae1d69934..2a5b2085828 100644 --- a/src/libutil/source-path.cc +++ b/src/libutil/source-path.cc @@ -62,44 +62,6 @@ bool SourcePath::operator<(const SourcePath & x) const return std::tie(*accessor, path) < std::tie(*x.accessor, x.path); } -SourcePath SourcePath::resolveSymlinks(SymlinkResolution mode) const -{ - auto res = SourcePath(accessor); - - int linksAllowed = 1024; - - std::list todo; - 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(); - if (c == "" || c == ".") - ; - else if (c == "..") - res.path.pop(); - else { - res.path.push(c); - 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, "/")); - } - } - } - } - - return res; -} - std::ostream & operator<<(std::ostream & str, const SourcePath & path) { str << path.to_string(); diff --git a/src/libutil/source-path.hh b/src/libutil/source-path.hh index 59991c64065..b8f69af126a 100644 --- a/src/libutil/source-path.hh +++ b/src/libutil/source-path.hh @@ -11,26 +11,6 @@ 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. - * - * 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 @@ -123,14 +103,13 @@ struct SourcePath bool operator<(const SourcePath & x) const; /** - * 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. + * Convenience wrapper around `SourceAccessor::resolveSymlinks()`. */ SourcePath resolveSymlinks( - SymlinkResolution mode = SymlinkResolution::Full) const; + SymlinkResolution mode = SymlinkResolution::Full) const + { + return {accessor, accessor->resolveSymlinks(path, mode)}; + } }; std::ostream & operator << (std::ostream & str, const SourcePath & path); diff --git a/src/nix/unix/run.cc b/src/nix/unix/run.cc index 02e809e5c9e..dfd8b643ce9 100644 --- a/src/nix/unix/run.cc +++ b/src/nix/unix/run.cc @@ -124,7 +124,8 @@ struct CmdShell : InstallablesCommand, MixEnvironment if (true) pathAdditions.push_back(store->printStorePath(path) + "/bin"); - auto propPath = CanonPath(store->printStorePath(path)) / "nix-support" / "propagated-user-env-packages"; + auto propPath = accessor->resolveSymlinks( + CanonPath(store->printStorePath(path)) / "nix-support" / "propagated-user-env-packages"); if (auto st = accessor->maybeLstat(propPath); st && st->type == SourceAccessor::tRegular) { for (auto & p : tokenizeString(accessor->readFile(propPath))) todo.push(store->parseStorePath(p)); diff --git a/tests/functional/shell-hello.nix b/tests/functional/shell-hello.nix index dfe66ef93c8..5c9b7a4d994 100644 --- a/tests/functional/shell-hello.nix +++ b/tests/functional/shell-hello.nix @@ -1,6 +1,6 @@ with import ./config.nix; -{ +rec { hello = mkDerivation { name = "hello"; outputs = [ "out" "dev" ]; @@ -24,6 +24,14 @@ with import ./config.nix; ''; }; + hello-symlink = mkDerivation { + name = "hello-symlink"; + buildCommand = + '' + ln -s ${hello} $out + ''; + }; + salve-mundi = mkDerivation { name = "salve-mundi"; outputs = [ "out" ]; diff --git a/tests/functional/shell.sh b/tests/functional/shell.sh index 8bbeabedf75..d8980192959 100644 --- a/tests/functional/shell.sh +++ b/tests/functional/shell.sh @@ -10,6 +10,8 @@ nix shell -f shell-hello.nix hello -c hello NixOS | grep 'Hello NixOS' nix shell -f shell-hello.nix hello^dev -c hello2 | grep 'Hello2' nix shell -f shell-hello.nix 'hello^*' -c hello2 | grep 'Hello2' +# Test output paths that are a symlink. +#nix shell -f shell-hello.nix hello-symlink -c hello | grep 'Hello World' if isDaemonNewer "2.20.0pre20231220"; then # Test that command line attribute ordering is reflected in the PATH From 9d50f57fa360df96a4f92c73db25dcec2a6f39d4 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 11 Apr 2024 09:00:47 +0200 Subject: [PATCH 2/3] Doh --- tests/functional/shell.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/functional/shell.sh b/tests/functional/shell.sh index d8980192959..abc091d92ea 100644 --- a/tests/functional/shell.sh +++ b/tests/functional/shell.sh @@ -11,7 +11,7 @@ nix shell -f shell-hello.nix hello^dev -c hello2 | grep 'Hello2' nix shell -f shell-hello.nix 'hello^*' -c hello2 | grep 'Hello2' # Test output paths that are a symlink. -#nix shell -f shell-hello.nix hello-symlink -c hello | grep 'Hello World' +nix shell -f shell-hello.nix hello-symlink -c hello | grep 'Hello World' if isDaemonNewer "2.20.0pre20231220"; then # Test that command line attribute ordering is reflected in the PATH From 26a4688a868e848760908ee15434eff2774952c3 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 11 Apr 2024 09:04:26 +0200 Subject: [PATCH 3/3] nix shell: Test that store paths cannot link outside of the store --- tests/functional/shell-hello.nix | 8 ++++++++ tests/functional/shell.sh | 3 +++ 2 files changed, 11 insertions(+) diff --git a/tests/functional/shell-hello.nix b/tests/functional/shell-hello.nix index 5c9b7a4d994..c46fdec8a8c 100644 --- a/tests/functional/shell-hello.nix +++ b/tests/functional/shell-hello.nix @@ -32,6 +32,14 @@ rec { ''; }; + forbidden-symlink = mkDerivation { + name = "forbidden-symlink"; + buildCommand = + '' + ln -s /tmp/foo/bar $out + ''; + }; + salve-mundi = mkDerivation { name = "salve-mundi"; outputs = [ "out" ]; diff --git a/tests/functional/shell.sh b/tests/functional/shell.sh index abc091d92ea..8a3fef3e7b1 100644 --- a/tests/functional/shell.sh +++ b/tests/functional/shell.sh @@ -13,6 +13,9 @@ nix shell -f shell-hello.nix 'hello^*' -c hello2 | grep 'Hello2' # Test output paths that are a symlink. nix shell -f shell-hello.nix hello-symlink -c hello | grep 'Hello World' +# Test that symlinks outside of the store don't work. +expect 1 nix shell -f shell-hello.nix forbidden-symlink -c hello 2>&1 | grepQuiet "is not in the Nix store" + if isDaemonNewer "2.20.0pre20231220"; then # Test that command line attribute ordering is reflected in the PATH # https://github.com/NixOS/nix/issues/7905