Eliminate useChroot

src/libstore/unix/build/darwin-derivation-builder.cc

@@ -1,5 +1,15 @@
 #ifdef __APPLE__
 
+#  include <spawn.h>
+#  include <sys/sysctl.h>
+#  include <sandbox.h>
+
+/* This definition is undocumented but depended upon by all major browsers. */
+extern "C" int
+sandbox_init_with_parameters(const char * profile, uint64_t flags, const char * const parameters[], char ** errorbuf);
+
+namespace nix {
+
 struct DarwinDerivationBuilder : DerivationBuilderImpl
 {
     PathsInChroot pathsInChroot;
@@ -185,4 +195,6 @@ struct DarwinDerivationBuilder : DerivationBuilderImpl
     }
 }
 
+}
+
 #endif

src/libstore/unix/build/derivation-builder.cc

@@ -52,15 +52,6 @@
 # include "nix/store/personality.hh"
 #endif
 
-#ifdef __APPLE__
-# include <spawn.h>
-# include <sys/sysctl.h>
-# include <sandbox.h>
-
-/* This definition is undocumented but depended upon by all major browsers. */
-extern "C" int sandbox_init_with_parameters(const char *profile, uint64_t flags, const char *const parameters[], char **errorbuf);
-#endif
-
 #include <pwd.h>
 #include <grp.h>
 #include <iostream>
@@ -116,6 +107,7 @@ protected:
     /**
      * The cgroup of the builder, if any.
      */
+    // FIXME: move
     std::optional<Path> cgroup;
 
     /**
@@ -134,18 +126,6 @@ protected:
      */
     Path tmpDirInSandbox;
 
-    /**
-     * Whether we're currently doing a chroot build.
-     */
-    // FIXME: remove
-    bool useChroot = false;
-
-    /**
-     * The root of the chroot environment.
-     */
-    // FIXME: move
-    Path chrootRootDir;
-
     /**
      * RAII object to delete the chroot directory.
      */
@@ -250,6 +230,14 @@ public:
 
 protected:
 
+    /**
+     * Acquire a build user lock. Return nullptr if no lock is available.
+     */
+    virtual std::unique_ptr<UserLock> getBuildUser()
+    {
+        return acquireUserLock(1, false);
+    }
+
     /**
      * Return the paths that should be made available in the sandbox.
      * This includes:
@@ -261,12 +249,28 @@ protected:
      */
     PathsInChroot getPathsInSandbox();
 
+    virtual void setBuildTmpDir()
+    {
+        tmpDir = topTmpDir;
+        tmpDirInSandbox = topTmpDir;
+    }
+
     /**
      * Called by prepareBuild() to do any setup in the parent to
      * prepare for a sandboxed build.
      */
     virtual void prepareSandbox();
 
+    virtual Strings getPreBuildHookArgs()
+    {
+        return Strings({store.printStorePath(drvPath)});
+    }
+
+    virtual Path realPathInSandbox(const Path & p)
+    {
+        return store.toRealPath(p);
+    }
+
     /**
      * Open the slave side of the pseudoterminal and use it as stderr.
      */
@@ -370,9 +374,13 @@ public:
 
     void killSandbox(bool getStats) override;
 
+protected:
+
+    virtual void cleanupBuild();
+
 private:
 
-    bool cleanupDecideWhetherDiskFull();
+    bool decideWhetherDiskFull();
 
     /**
      * Create alternative path calculated from but distinct from the
@@ -462,12 +470,11 @@ bool DerivationBuilderImpl::prepareBuild()
 {
     if (useBuildUsers()) {
         if (!buildUser)
-            buildUser = acquireUserLock(drvOptions.useUidRange(drv) ? 65536 : 1, useChroot);
+            buildUser = getBuildUser();
 
-        if (!buildUser) {
+        if (!buildUser)
             return false;
     }
-    }
 
     return true;
 }
@@ -528,7 +535,9 @@ std::variant<std::pair<BuildResult::Status, Error>, SingleDrvOutputs> Derivation
         /* Check the exit status. */
         if (!statusOk(status)) {
 
-            diskFull |= cleanupDecideWhetherDiskFull();
+            diskFull |= decideWhetherDiskFull();
+
+            cleanupBuild();
 
             auto msg = fmt(
                 "Cannot build '%s'.\n"
@@ -582,6 +591,10 @@ std::variant<std::pair<BuildResult::Status, Error>, SingleDrvOutputs> Derivation
     }
 }
 
+void DerivationBuilderImpl::cleanupBuild()
+{
+    deleteTmpDir(false);
+}
 
 static void chmod_(const Path & path, mode_t mode)
 {
@@ -644,10 +657,7 @@ static void replaceValidPath(const Path & storePath, const Path & tmpPath)
         deletePath(oldPath);
 }
 
-
+bool DerivationBuilderImpl::decideWhetherDiskFull()
-
-
-bool DerivationBuilderImpl::cleanupDecideWhetherDiskFull()
 {
     bool diskFull = false;
 
@@ -670,19 +680,6 @@ bool DerivationBuilderImpl::cleanupDecideWhetherDiskFull()
     }
 #endif
 
-    deleteTmpDir(false);
-
-    /* Move paths out of the chroot for easier debugging of
-       build failures. */
-    if (useChroot && buildMode == bmNormal)
-        for (auto & [_, status] : initialOutputs) {
-            if (!status.known) continue;
-            if (buildMode != bmCheck && status.known->isValid()) continue;
-            auto p = store.toRealPath(status.known->path);
-            if (pathExists(chrootRootDir + p))
-                std::filesystem::rename((chrootRootDir + p), p);
-        }
-
     return diskFull;
 }
 
@@ -832,23 +829,9 @@ void DerivationBuilderImpl::startBuilder()
     /* Create a temporary directory where the build will take
        place. */
     topTmpDir = createTempDir(settings.buildDir.get().value_or(""), "nix-build-" + std::string(drvPath.name()), 0700);
-#ifdef __APPLE__
+    setBuildTmpDir();
-    if (false) {
+    assert(!tmpDir.empty());
-#else
+    assert(!tmpDirInSandbox.empty());
-    if (useChroot) {
-#endif
-        /* If sandboxing is enabled, put the actual TMPDIR underneath
-           an inaccessible root-owned directory, to prevent outside
-           access.
-
-           On macOS, we don't use an actual chroot, so this isn't
-           possible. Any mitigation along these lines would have to be
-           done directly in the sandbox profile. */
-        tmpDir = topTmpDir + "/build";
-        createDir(tmpDir, 0700);
-    } else {
-        tmpDir = topTmpDir;
-    }
     chownToBuilder(tmpDir);
 
     for (auto & [outputName, status] : initialOutputs) {
@@ -1053,14 +1036,12 @@ DerivationBuilderImpl::PathsInChroot DerivationBuilderImpl::getPathsInSandbox()
 
     if (settings.preBuildHook != "") {
         printMsg(lvlChatty, "executing pre-build hook '%1%'", settings.preBuildHook);
-        auto args = useChroot ? Strings({store.printStorePath(drvPath), chrootRootDir}) :
-            Strings({ store.printStorePath(drvPath) });
         enum BuildHookState {
             stBegin,
             stExtraChrootDirs
         };
         auto state = stBegin;
-        auto lines = runProgram(settings.preBuildHook, false, args);
+        auto lines = runProgram(settings.preBuildHook, false, getPreBuildHookArgs());
         auto lastPos = std::string::size_type{0};
         for (auto nlPos = lines.find('\n'); nlPos != std::string::npos;
                 nlPos = lines.find('\n', lastPos))
@@ -1157,14 +1138,6 @@ void DerivationBuilderImpl::processSandboxSetupMessages()
 
 void DerivationBuilderImpl::initTmpDir()
 {
-    /* In a sandbox, for determinism, always use the same temporary
-       directory. */
-#ifdef __linux__
-    tmpDirInSandbox = useChroot ? settings.sandboxBuildDir : tmpDir;
-#else
-    tmpDirInSandbox = tmpDir;
-#endif
-
     /* In non-structured mode, set all bindings either directory in the
        environment or via a file, as specified by
        `DerivationOptions::passAsFile`. */
@@ -1653,14 +1626,6 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
     for (auto & i : scratchOutputs) referenceablePaths.insert(i.second);
     for (auto & p : addedPaths) referenceablePaths.insert(p);
 
-    /* FIXME `needsHashRewrite` should probably be removed and we get to the
-       real reason why we aren't using the chroot dir */
-    auto toRealPathChroot = [&](const Path & p) -> Path {
-        return useChroot && !needsHashRewrite()
-            ? chrootRootDir + p
-            : store.toRealPath(p);
-    };
-
     /* Check whether the output paths were created, and make all
        output paths read-only.  Then get the references of each output (that we
        might need to register), so we can topologically sort them. For the ones
@@ -1677,7 +1642,7 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
             throw BuildError(
                 "builder for '%s' has no scratch output for '%s'",
                 store.printStorePath(drvPath), outputName);
-        auto actualPath = toRealPathChroot(store.printStorePath(*scratchOutput));
+        auto actualPath = realPathInSandbox(store.printStorePath(*scratchOutput));
 
         outputsToSort.insert(outputName);
 
@@ -1786,7 +1751,7 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
         auto output = get(drv.outputs, outputName);
         auto scratchPath = get(scratchOutputs, outputName);
         assert(output && scratchPath);
-        auto actualPath = toRealPathChroot(store.printStorePath(*scratchPath));
+        auto actualPath = realPathInSandbox(store.printStorePath(*scratchPath));
 
         auto finish = [&](StorePath finalStorePath) {
             /* Store the final path */
@@ -2361,10 +2326,14 @@ StorePath DerivationBuilderImpl::makeFallbackPath(const StorePath & path)
         Hash(HashAlgorithm::SHA256), path.name());
 }
 
+}
+
 // FIXME: do this properly
 #include "linux-derivation-builder.cc"
 #include "darwin-derivation-builder.cc"
 
+namespace nix {
+
 std::unique_ptr<DerivationBuilder> makeDerivationBuilder(
     Store & store,
     std::unique_ptr<DerivationBuilderCallbacks> miscMethods,

src/libstore/unix/build/linux-derivation-builder.cc

@@ -1,5 +1,7 @@
 #ifdef __linux__
 
+namespace nix {
+
 struct LinuxDerivationBuilder : DerivationBuilderImpl
 {
     /**
@@ -20,23 +22,56 @@ struct LinuxDerivationBuilder : DerivationBuilderImpl
      */
     bool usingUserNamespace = true;
 
+    /**
+     * The root of the chroot environment.
+     */
+    Path chrootRootDir;
+
     PathsInChroot pathsInChroot;
 
     LinuxDerivationBuilder(
         Store & store, std::unique_ptr<DerivationBuilderCallbacks> miscMethods, DerivationBuilderParams params)
         : DerivationBuilderImpl(store, std::move(miscMethods), std::move(params))
     {
-        useChroot = true;
     }
 
-    uid_t sandboxUid() { return usingUserNamespace ? (!buildUser || buildUser->getUIDCount() == 1 ? 1000 : 0) : buildUser->getUID(); }
+    uid_t sandboxUid()
-    gid_t sandboxGid() { return usingUserNamespace ? (!buildUser || buildUser->getUIDCount() == 1 ? 100  : 0) : buildUser->getGID(); }
+    {
+        return usingUserNamespace ? (!buildUser || buildUser->getUIDCount() == 1 ? 1000 : 0) : buildUser->getUID();
+    }
+
+    gid_t sandboxGid()
+    {
+        return usingUserNamespace ? (!buildUser || buildUser->getUIDCount() == 1 ? 100 : 0) : buildUser->getGID();
+    }
 
     bool needsHashRewrite() override
     {
         return false;
     }
 
+    std::unique_ptr<UserLock> getBuildUser() override
+    {
+        return acquireUserLock(drvOptions.useUidRange(drv) ? 65536 : 1, true);
+    }
+
+    void setBuildTmpDir() override
+    {
+        /* If sandboxing is enabled, put the actual TMPDIR underneath
+           an inaccessible root-owned directory, to prevent outside
+           access.
+
+           On macOS, we don't use an actual chroot, so this isn't
+           possible. Any mitigation along these lines would have to be
+           done directly in the sandbox profile. */
+        tmpDir = topTmpDir + "/build";
+        createDir(tmpDir, 0700);
+
+        /* In a sandbox, for determinism, always use the same temporary
+           directory. */
+        tmpDirInSandbox = settings.sandboxBuildDir;
+    }
+
     void prepareSandbox() override
     {
         /* Create a temporary directory in which we set up the chroot
@@ -59,7 +94,10 @@ struct LinuxDerivationBuilder : DerivationBuilderImpl
         if (mkdir(chrootRootDir.c_str(), buildUser && buildUser->getUIDCount() != 1 ? 0755 : 0750) == -1)
             throw SysError("cannot create '%1%'", chrootRootDir);
 
-        if (buildUser && chown(chrootRootDir.c_str(), buildUser->getUIDCount() != 1 ? buildUser->getUID() : 0, buildUser->getGID()) == -1)
+        if (buildUser
+            && chown(
+                   chrootRootDir.c_str(), buildUser->getUIDCount() != 1 ? buildUser->getUID() : 0, buildUser->getGID())
+                   == -1)
             throw SysError("cannot change ownership of '%1%'", chrootRootDir);
 
         /* Create a writable /tmp in the chroot.  Many builders need
@@ -81,10 +119,12 @@ struct LinuxDerivationBuilder : DerivationBuilderImpl
 
         /* Declare the build user's group so that programs get a consistent
            view of the system (e.g., "id -gn"). */
-        writeFile(chrootRootDir + "/etc/group",
+        writeFile(
+            chrootRootDir + "/etc/group",
             fmt("root:x:0:\n"
                 "nixbld:!:%1%:\n"
-                "nogroup:x:65534:\n", sandboxGid()));
+                "nogroup:x:65534:\n",
+                sandboxGid()));
 
         /* Create /etc/hosts with localhost entry. */
         if (derivationType.isSandboxed())
@@ -125,7 +165,7 @@ struct LinuxDerivationBuilder : DerivationBuilderImpl
             chownToBuilder(*cgroup);
             chownToBuilder(*cgroup + "/cgroup.procs");
             chownToBuilder(*cgroup + "/cgroup.threads");
-            //chownToBuilder(*cgroup + "/cgroup.subtree_control");
+            // chownToBuilder(*cgroup + "/cgroup.subtree_control");
         }
 
         pathsInChroot = getPathsInSandbox();
@@ -136,6 +176,18 @@ struct LinuxDerivationBuilder : DerivationBuilderImpl
         }
     }
 
+    Strings getPreBuildHookArgs() override
+    {
+        assert(!chrootRootDir.empty());
+        return Strings({store.printStorePath(drvPath), chrootRootDir});
+    }
+
+    Path realPathInSandbox(const Path & p) override
+    {
+        // FIXME: why the needsHashRewrite() conditional?
+        return !needsHashRewrite() ? chrootRootDir + p : store.toRealPath(p);
+    }
+
     void startChild() override
     {
         /* Set up private namespaces for the build:
@@ -194,7 +246,8 @@ struct LinuxDerivationBuilder : DerivationBuilderImpl
                     if (errno != EPERM)
                         throw SysError("setgroups failed");
                     if (settings.requireDropSupplementaryGroups)
-                        throw Error("setgroups failed. Set the require-drop-supplementary-groups option to false to skip this step.");
+                        throw Error(
+                            "setgroups failed. Set the require-drop-supplementary-groups option to false to skip this step.");
                 }
 
                 ProcessOptions options;
@@ -226,9 +279,7 @@ struct LinuxDerivationBuilder : DerivationBuilderImpl
 
         /* Close the write side to prevent runChild() from hanging
            reading from this. */
-        Finally cleanup([&]() {
+        Finally cleanup([&]() { userNamespaceSync.writeSide = -1; });
-            userNamespaceSync.writeSide = -1;
-        });
 
         auto ss = tokenizeString<std::vector<std::string>>(readLine(sendPid.readSide.get()));
         assert(ss.size() == 1);
@@ -242,27 +293,29 @@ struct LinuxDerivationBuilder : DerivationBuilderImpl
             uid_t hostGid = buildUser ? buildUser->getGID() : getgid();
             uid_t nrIds = buildUser ? buildUser->getUIDCount() : 1;
 
-            writeFile("/proc/" + std::to_string(pid) + "/uid_map",
+            writeFile("/proc/" + std::to_string(pid) + "/uid_map", fmt("%d %d %d", sandboxUid(), hostUid, nrIds));
-                fmt("%d %d %d", sandboxUid(), hostUid, nrIds));
 
             if (!buildUser || buildUser->getUIDCount() == 1)
                 writeFile("/proc/" + std::to_string(pid) + "/setgroups", "deny");
 
-            writeFile("/proc/" + std::to_string(pid) + "/gid_map",
+            writeFile("/proc/" + std::to_string(pid) + "/gid_map", fmt("%d %d %d", sandboxGid(), hostGid, nrIds));
-                fmt("%d %d %d", sandboxGid(), hostGid, nrIds));
         } else {
             debug("note: not using a user namespace");
             if (!buildUser)
-                throw Error("cannot perform a sandboxed build because user namespaces are not enabled; check /proc/sys/user/max_user_namespaces");
+                throw Error(
+                    "cannot perform a sandboxed build because user namespaces are not enabled; check /proc/sys/user/max_user_namespaces");
         }
 
         /* Now that we now the sandbox uid, we can write
            /etc/passwd. */
-        writeFile(chrootRootDir + "/etc/passwd", fmt(
+        writeFile(
-                "root:x:0:0:Nix build user:%3%:/noshell\n"
+            chrootRootDir + "/etc/passwd",
+            fmt("root:x:0:0:Nix build user:%3%:/noshell\n"
                 "nixbld:x:%1%:%2%:Nix build user:%3%:/noshell\n"
                 "nobody:x:65534:65534:Nobody:/:/noshell\n",
-                sandboxUid(), sandboxGid(), settings.sandboxBuildDir));
+                sandboxUid(),
+                sandboxGid(),
+                settings.sandboxBuildDir));
 
         /* Save the mount- and user namespace of the child. We have to do this
          *before* the child does a chroot. */
@@ -528,6 +581,24 @@ struct LinuxDerivationBuilder : DerivationBuilderImpl
         return DerivationBuilderImpl::unprepareBuild();
     }
 
+    void cleanupBuild() override
+    {
+        DerivationBuilderImpl::cleanupBuild();
+
+        /* Move paths out of the chroot for easier debugging of
+           build failures. */
+        if (buildMode == bmNormal)
+            for (auto & [_, status] : initialOutputs) {
+                if (!status.known)
+                    continue;
+                if (buildMode != bmCheck && status.known->isValid())
+                    continue;
+                auto p = store.toRealPath(status.known->path);
+                if (pathExists(chrootRootDir + p))
+                    std::filesystem::rename((chrootRootDir + p), p);
+            }
+    }
+
     void addDependency(const StorePath & path) override
     {
         if (isAllowed(path))
@@ -568,4 +639,6 @@ struct LinuxDerivationBuilder : DerivationBuilderImpl
     }
 };
 
+}
+
 #endif