Merge fc3f70cd5e into f51974d698

Fix `Interrupted` falling out of thread crash

src/libexpr/eval.cc

@@ -247,12 +247,14 @@ EvalState::EvalState(
     , emptyBindings(0)
     , rootFS(
         settings.restrictEval || settings.pureEval
-        ? ref<SourceAccessor>(AllowListSourceAccessor::create(getFSSourceAccessor(), {},
+        ? ref<SourceAccessor>(AllowListSourceAccessor::create(getFSSourceAccessor(), {}, {},
             [&settings](const CanonPath & path) -> RestrictedPathError {
-                auto modeInformation = settings.pureEval
-                    ? "in pure evaluation mode (use '--impure' to override)"
-                    : "in restricted mode";
-                throw RestrictedPathError("access to absolute path '%1%' is forbidden %2%", path, modeInformation);
+                return RestrictedPathError(
+                    std::string("access to absolute path '%1%' is forbidden ") +
+                    (settings.pureEval
+                        ? "in pure evaluation mode (use '--impure' to override)"
+                        : "in restricted mode"),
+                    path);
             }))
         : getFSSourceAccessor())
     , corepkgsFS(make_ref<MemorySourceAccessor>())

src/libfetchers/filtering-source-accessor.cc

@@ -1,5 +1,7 @@
 #include "filtering-source-accessor.hh"
 
+#include <unordered_set>
+
 namespace nix {
 
 std::optional<std::filesystem::path> FilteringSourceAccessor::getPhysicalPath(const CanonPath & path)
@@ -19,10 +21,15 @@ bool FilteringSourceAccessor::pathExists(const CanonPath & path)
     return isAllowed(path) && next->pathExists(prefix / path);
 }
 
-std::optional<SourceAccessor::Stat> FilteringSourceAccessor::maybeLstat(const CanonPath & path)
+SourceAccessor::Stat FilteringSourceAccessor::lstat(const CanonPath & path)
 {
     checkAccess(path);
-    return next->maybeLstat(prefix / path);
+    return next->lstat(prefix / path);
+}
+
+std::optional<SourceAccessor::Stat> FilteringSourceAccessor::maybeLstat(const CanonPath & path)
+{
+    return isAllowed(path) ? next->maybeLstat(prefix / path) : std::nullopt;
 }
 
 SourceAccessor::DirEntries FilteringSourceAccessor::readDirectory(const CanonPath & path)
@@ -57,33 +64,46 @@ void FilteringSourceAccessor::checkAccess(const CanonPath & path)
 
 struct AllowListSourceAccessorImpl : AllowListSourceAccessor
 {
-    std::set<CanonPath> allowedPrefixes;
+    std::unordered_set<CanonPath> allowedPrefixes;
+    std::unordered_set<CanonPath> allowedPaths;
 
     AllowListSourceAccessorImpl(
         ref<SourceAccessor> next,
-        std::set<CanonPath> && allowedPrefixes,
+        std::unordered_set<CanonPath> && allowedPrefixes,
+        std::unordered_set<CanonPath> && allowedPaths,
         MakeNotAllowedError && makeNotAllowedError)
         : AllowListSourceAccessor(SourcePath(next), std::move(makeNotAllowedError))
         , allowedPrefixes(std::move(allowedPrefixes))
+        , allowedPaths(std::move(allowedPaths))
     { }
 
     bool isAllowed(const CanonPath & path) override
     {
-        return path.isAllowed(allowedPrefixes);
+        return allowedPaths.count(path) || path.isAllowed(allowedPrefixes);
     }
 
     void allowPrefix(CanonPath prefix) override
     {
         allowedPrefixes.insert(std::move(prefix));
     }
+
+    void allowPath(CanonPath path) override
+    {
+        allowedPaths.insert(std::move(path));
+    }
 };
 
 ref<AllowListSourceAccessor> AllowListSourceAccessor::create(
     ref<SourceAccessor> next,
-    std::set<CanonPath> && allowedPrefixes,
+    std::unordered_set<CanonPath> && allowedPrefixes,
+    std::unordered_set<CanonPath> && allowedPaths,
     MakeNotAllowedError && makeNotAllowedError)
 {
-    return make_ref<AllowListSourceAccessorImpl>(next, std::move(allowedPrefixes), std::move(makeNotAllowedError));
+    return make_ref<AllowListSourceAccessorImpl>(
+        next,
+        std::move(allowedPrefixes),
+        std::move(allowedPaths),
+        std::move(makeNotAllowedError));
 }
 
 bool CachingFilteringSourceAccessor::isAllowed(const CanonPath & path)

src/libfetchers/filtering-source-accessor.hh

@@ -36,6 +36,8 @@ struct FilteringSourceAccessor : SourceAccessor
 
     bool pathExists(const CanonPath & path) override;
 
+    Stat lstat(const CanonPath & path) override;
+
     std::optional<Stat> maybeLstat(const CanonPath & path) override;
 
     DirEntries readDirectory(const CanonPath & path) override;
@@ -63,13 +65,20 @@ struct FilteringSourceAccessor : SourceAccessor
 struct AllowListSourceAccessor : public FilteringSourceAccessor
 {
     /**
-     * Grant access to the specified prefix.
+     * Grant access to the specified prefix, i.e. the path *and* its
+     * children.
      */
     virtual void allowPrefix(CanonPath prefix) = 0;
 
+    /**
+     * Grant access to a path but not its children.
+     */
+    virtual void allowPath(CanonPath path) = 0;
+
     static ref<AllowListSourceAccessor> create(
         ref<SourceAccessor> next,
-        std::set<CanonPath> && allowedPrefixes,
+        std::unordered_set<CanonPath> && allowedPrefixes,
+        std::unordered_set<CanonPath> && allowedPaths,
         MakeNotAllowedError && makeNotAllowedError);
 
     using FilteringSourceAccessor::FilteringSourceAccessor;

src/libfetchers/git-utils.cc

@@ -1201,17 +1201,23 @@ ref<SourceAccessor> GitRepoImpl::getAccessor(const Hash & rev, bool exportIgnore
 ref<SourceAccessor> GitRepoImpl::getAccessor(const WorkdirInfo & wd, bool exportIgnore, MakeNotAllowedError makeNotAllowedError)
 {
     auto self = ref<GitRepoImpl>(shared_from_this());
-    /* In case of an empty workdir, return an empty in-memory tree. We
-       cannot use AllowListSourceAccessor because it would return an
-       error for the root (and we can't add the root to the allow-list
-       since that would allow access to all its children). */
-    ref<SourceAccessor> fileAccessor =
-        wd.files.empty()
-        ? makeEmptySourceAccessor()
-        : AllowListSourceAccessor::create(
-            makeFSSourceAccessor(path),
-            std::set<CanonPath> { wd.files },
-            std::move(makeNotAllowedError)).cast<SourceAccessor>();
+
+    /* Grant access to the parent directories of every accessible
+       file. The root is always accessible. */
+    std::unordered_set<CanonPath> files{CanonPath::root};
+    for (auto path : wd.files) {
+        while (!path.isRoot()) {
+            if (!files.insert(path).second) break;
+            path.pop();
+        }
+    }
+
+    auto fileAccessor = AllowListSourceAccessor::create(
+        makeFSSourceAccessor(path),
+        {},
+        std::move(files),
+        std::move(makeNotAllowedError));
+
     if (exportIgnore)
         return make_ref<GitExportIgnoreSourceAccessor>(self, fileAccessor, std::nullopt);
     else

src/libstore/unix/build/local-derivation-goal.cc

@@ -65,6 +65,7 @@
 #include <iostream>
 
 #include "strings.hh"
+#include "signals.hh"
 
 namespace nix {
 
@@ -1579,6 +1580,8 @@ void LocalDerivationGoal::startDaemon()
                         FdSink(remote.get()),
                         NotTrusted, daemon::Recursive);
                     debug("terminated daemon connection");
+                } catch (const Interrupted &) {
+                    debug("interrupted daemon connection");
                 } catch (SystemError &) {
                     ignoreExceptionExceptInterrupt();
                 }

src/libstore/unix/build/local-derivation-goal.hh

@@ -225,8 +225,15 @@ struct LocalDerivationGoal : public DerivationGoal
      */
     void writeStructuredAttrs();
 
+    /**
+     * Start an in-process nix daemon thread for recursive-nix.
+     */
     void startDaemon();
 
+    /**
+     * Stop the in-process nix daemon thread.
+     * @see startDaemon
+     */
     void stopDaemon();
 
     /**

src/libutil/canon-path.cc

@@ -90,25 +90,16 @@ CanonPath CanonPath::operator / (std::string_view c) const
     return res;
 }
 
-bool CanonPath::isAllowed(const std::set<CanonPath> & allowed) const
+bool CanonPath::isAllowed(const std::unordered_set<CanonPath> & allowed) const
 {
-    /* Check if `this` is an exact match or the parent of an
-       allowed path. */
-    auto lb = allowed.lower_bound(*this);
-    if (lb != allowed.end()) {
-        if (lb->isWithin(*this))
-            return true;
-    }
-
-    /* Check if a parent of `this` is allowed. */
     auto path = *this;
-    while (!path.isRoot()) {
-        path.pop();
+    while (true) {
         if (allowed.count(path))
             return true;
+        if (path.isRoot())
+            return false;
+        path.pop();
     }
-
-    return false;
 }
 
 std::ostream & operator << (std::ostream & stream, const CanonPath & path)

src/libutil/canon-path.hh

@@ -5,7 +5,7 @@
 #include <optional>
 #include <cassert>
 #include <iostream>
-#include <set>
+#include <unordered_set>
 #include <vector>
 
 namespace nix {
@@ -209,12 +209,10 @@ public:
     CanonPath operator / (std::string_view c) const;
 
     /**
-     * Check whether access to this path is allowed, which is the case
-     * if 1) `this` is within any of the `allowed` paths; or 2) any of
-     * the `allowed` paths are within `this`. (The latter condition
-     * ensures access to the parents of allowed paths.)
+     * Check whether access to this path is allowed, i.e. `this` is
+     * within any of the `allowed` paths.
      */
-    bool isAllowed(const std::set<CanonPath> & allowed) const;
+    bool isAllowed(const std::unordered_set<CanonPath> & allowed) const;
 
     /**
      * Return a representation `x` of `path` relative to `this`, i.e.

src/libutil/source-accessor.hh

@@ -114,7 +114,7 @@ struct SourceAccessor : std::enable_shared_from_this<SourceAccessor>
         std::optional<uint64_t> narOffset;
     };
 
-    Stat lstat(const CanonPath & path);
+    virtual Stat lstat(const CanonPath & path);
 
     virtual std::optional<Stat> maybeLstat(const CanonPath & path) = 0;
 

src/libutil/thread-pool.cc

@@ -110,10 +110,15 @@ void ThreadPool::doWork(bool mainThread)
                            propagate it. */
                         try {
                             std::rethrow_exception(exc);
+                        } catch (const Interrupted &) {
+                            // The interrupted state may be picked up by multiple
+                            // workers, which is expected, so we should ignore
+                            // it silently and let the first one bubble up,
+                            // rethrown via the original state->exception.
+                        } catch (const ThreadPoolShutDown &) {
+                            // Similarly expected.
                         } catch (std::exception & e) {
-                            if (!dynamic_cast<ThreadPoolShutDown*>(&e))
-                                ignoreExceptionExceptInterrupt();
-                        } catch (...) {
+                            ignoreExceptionExceptInterrupt();
                         }
                     }
                 }

tests/functional/flakes/flakes.sh

@@ -42,6 +42,7 @@ cat > "$flake2Dir/flake.nix" <<EOF
 
   outputs = { self, flake1 }: rec {
     packages.$system.bar = flake1.packages.$system.foo;
+    foo = builtins.pathExists (self + "/..");
   };
 }
 EOF
@@ -274,6 +275,9 @@ nix build -o "$TEST_ROOT/result" "$flake2Dir#bar" --commit-lock-file
 [[ -e "$flake2Dir/flake.lock" ]]
 [[ -z $(git -C "$flake2Dir" diff main || echo failed) ]]
 
+# Test that pathExist on the parent of a flake returns false.
+[[ $(nix eval "$flake2Dir#foo") = false ]]
+
 # Rerunning the build should not change the lockfile.
 nix build -o "$TEST_ROOT/result" "$flake2Dir#bar"
 [[ -z $(git -C "$flake2Dir" diff main || echo failed) ]]

tests/functional/repl.sh

@@ -153,7 +153,7 @@ foo + baz
 ' "3" \
     ./flake ./flake\#bar --experimental-features 'flakes'
 
-# Test the `:reload` mechansim with flakes:
+# Test the `:reload` mechanism with flakes:
 # - Eval `./flake#changingThing`
 # - Modify the flake
 # - Re-eval it

tests/functional/restricted.sh

@@ -5,15 +5,15 @@ source common.sh
 clearStoreIfPossible
 
 nix-instantiate --restrict-eval --eval -E '1 + 2'
-(! nix-instantiate --eval --restrict-eval ./restricted.nix)
-(! nix-instantiate --eval --restrict-eval <(echo '1 + 2'))
+expectStderr 1 nix-instantiate --eval --restrict-eval ./restricted.nix | grepQuiet "forbidden in restricted mode"
+expectStderr 1 nix-instantiate --eval --restrict-eval <(echo '1 + 2') | grepQuiet "forbidden in restricted mode"
 nix-instantiate --restrict-eval ./simple.nix -I src=.
 nix-instantiate --restrict-eval ./simple.nix -I src1=simple.nix -I src2=config.nix -I src3=./simple.builder.sh
 
 # no default NIX_PATH
-(unset NIX_PATH; ! nix-instantiate --restrict-eval --find-file .)
+(unset NIX_PATH; expectStderr 1 nix-instantiate --restrict-eval --find-file . | grepQuiet "file '.' was not found in the Nix search path")
 
-(! nix-instantiate --restrict-eval --eval -E 'builtins.readFile ./simple.nix')
+expectStderr 1 nix-instantiate --restrict-eval --eval -E 'builtins.readFile ./simple.nix' | grepQuiet "forbidden in restricted mode"
 nix-instantiate --restrict-eval --eval -E 'builtins.readFile ./simple.nix' -I src=../..
 
 expectStderr 1 nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in builtins.readFile <foo/simple.nix>' | grepQuiet "forbidden in restricted mode"
@@ -22,21 +22,21 @@ nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; p
 p=$(nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval --allowed-uris "file://$(pwd)")
 cmp $p restricted.sh
 
-(! nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval)
+expectStderr 1 nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval | grepQuiet "forbidden in restricted mode"
 
-(! nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval --allowed-uris "file://$(pwd)/restricted.sh/")
+expectStderr 1 nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval --allowed-uris "file://$(pwd)/restricted.sh/" | grepQuiet "forbidden in restricted mode"
 
 nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval --allowed-uris "file://$(pwd)/restricted.sh"
 
-(! nix eval --raw --expr "builtins.fetchurl https://github.com/NixOS/patchelf/archive/master.tar.gz" --impure --restrict-eval)
-(! nix eval --raw --expr "builtins.fetchTarball https://github.com/NixOS/patchelf/archive/master.tar.gz" --impure --restrict-eval)
-(! nix eval --raw --expr "fetchGit git://github.com/NixOS/patchelf.git" --impure --restrict-eval)
+expectStderr 1 nix eval --raw --expr "builtins.fetchurl https://github.com/NixOS/patchelf/archive/master.tar.gz" --impure --restrict-eval | grepQuiet "forbidden in restricted mode"
+expectStderr 1 nix eval --raw --expr "builtins.fetchTarball https://github.com/NixOS/patchelf/archive/master.tar.gz" --impure --restrict-eval | grepQuiet "forbidden in restricted mode"
+expectStderr 1 nix eval --raw --expr "fetchGit git://github.com/NixOS/patchelf.git" --impure --restrict-eval | grepQuiet "forbidden in restricted mode"
 
 ln -sfn $(pwd)/restricted.nix $TEST_ROOT/restricted.nix
 [[ $(nix-instantiate --eval $TEST_ROOT/restricted.nix) == 3 ]]
-(! nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix)
-(! nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix -I $TEST_ROOT)
-(! nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix -I .)
+expectStderr 1 nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix | grepQuiet "forbidden in restricted mode"
+expectStderr 1 nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix -I $TEST_ROOT | grepQuiet "forbidden in restricted mode"
+expectStderr 1 nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix -I . | grepQuiet "forbidden in restricted mode"
 nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix -I $TEST_ROOT -I .
 
 [[ $(nix eval --raw --impure --restrict-eval -I . --expr 'builtins.readFile "${import ./simple.nix}/hello"') == 'Hello World!' ]]
@@ -50,8 +50,8 @@ expectStderr 1 nix-instantiate --restrict-eval --eval -E "let __nixPath = [ { pr
 
 expectStderr 1 nix-instantiate --restrict-eval --eval -E "let __nixPath = [ { prefix = \"foo\"; path = $TEST_ROOT/tunnel.d; } ]; in builtins.readDir <foo/tunnel/foo2>" -I $TEST_ROOT/tunnel.d | grepQuiet "forbidden in restricted mode"
 
-# Reading the parents of allowed paths should show only the ancestors of the allowed paths.
-[[ $(nix-instantiate --restrict-eval --eval -E "let __nixPath = [ { prefix = \"foo\"; path = $TEST_ROOT/tunnel.d; } ]; in builtins.readDir <foo/tunnel>" -I $TEST_ROOT/tunnel.d) == '{ "tunnel.d" = "directory"; }' ]]
+# Reading the parents of allowed paths is forbidden.
+expectStderr 1 nix-instantiate --restrict-eval --eval -E "let __nixPath = [ { prefix = \"foo\"; path = $TEST_ROOT/tunnel.d; } ]; in builtins.readDir <foo/tunnel>" -I $TEST_ROOT/tunnel.d | grepQuiet "forbidden in restricted mode"
 
 # Check whether we can leak symlink information through directory traversal.
 traverseDir="$(pwd)/restricted-traverse-me"
@@ -63,5 +63,3 @@ output="$(nix eval --raw --restrict-eval -I "$traverseDir" \
     2>&1 || :)"
 echo "$output" | grep "is forbidden"
 echo "$output" | grepInverse -F restricted-secret
-
-expectStderr 1 nix-instantiate --restrict-eval true ./dependencies.nix | grepQuiet "forbidden in restricted mode"

tests/unit/libutil/canon-path.cc

@@ -143,7 +143,7 @@ namespace nix {
     }
 
     TEST(CanonPath, allowed) {
-        std::set<CanonPath> allowed {
+        std::unordered_set<CanonPath> allowed {
             CanonPath("foo/bar"),
             CanonPath("foo!"),
             CanonPath("xyzzy"),
@@ -152,11 +152,11 @@ namespace nix {
 
         ASSERT_TRUE (CanonPath("foo/bar").isAllowed(allowed));
         ASSERT_TRUE (CanonPath("foo/bar/bla").isAllowed(allowed));
-        ASSERT_TRUE (CanonPath("foo").isAllowed(allowed));
+        ASSERT_FALSE(CanonPath("foo").isAllowed(allowed));
         ASSERT_FALSE(CanonPath("bar").isAllowed(allowed));
         ASSERT_FALSE(CanonPath("bar/a").isAllowed(allowed));
-        ASSERT_TRUE (CanonPath("a").isAllowed(allowed));
-        ASSERT_TRUE (CanonPath("a/b").isAllowed(allowed));
+        ASSERT_FALSE(CanonPath("a").isAllowed(allowed));
+        ASSERT_FALSE(CanonPath("a/b").isAllowed(allowed));
         ASSERT_TRUE (CanonPath("a/b/c").isAllowed(allowed));
         ASSERT_TRUE (CanonPath("a/b/c/d").isAllowed(allowed));
         ASSERT_TRUE (CanonPath("a/b/c/d/e").isAllowed(allowed));
@@ -164,7 +164,7 @@ namespace nix {
         ASSERT_FALSE(CanonPath("a/b/d").isAllowed(allowed));
         ASSERT_FALSE(CanonPath("aaa").isAllowed(allowed));
         ASSERT_FALSE(CanonPath("zzz").isAllowed(allowed));
-        ASSERT_TRUE (CanonPath("/").isAllowed(allowed));
+        ASSERT_FALSE(CanonPath("/").isAllowed(allowed));
     }
 
     TEST(CanonPath, makeRelative) {