From c78686e411e0a14cff51836fe6c35d7584171df3 Mon Sep 17 00:00:00 2001 From: Graham Christensen Date: Fri, 10 May 2019 16:39:31 -0400 Subject: build: run diff-hook under --check and document diff-hook --- src/libstore/build.cc | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) (limited to 'src') diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 91eb97dfb873..026828c535ca 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -461,6 +461,19 @@ static void commonChildInit(Pipe & logPipe) close(fdDevNull); } +void handleDiffHook(Path tryA, Path tryB, Path drvPath) +{ + auto diffHook = settings.diffHook; + if (diffHook != "" && settings.runDiffHook) { + try { + auto diff = runProgram(diffHook, true, {tryA, tryB, drvPath}); + if (diff != "") + printError(chomp(diff)); + } catch (Error & error) { + printError("diff hook execution failed: %s", error.what()); + } + } +} ////////////////////////////////////////////////////////////////////// @@ -3039,8 +3052,7 @@ void DerivationGoal::registerOutputs() InodesSeen inodesSeen; Path checkSuffix = ".check"; - bool runDiffHook = settings.runDiffHook; - bool keepPreviousRound = settings.keepFailed || runDiffHook; + bool keepPreviousRound = settings.keepFailed || settings.runDiffHook; std::exception_ptr delayedException; @@ -3185,11 +3197,14 @@ void DerivationGoal::registerOutputs() if (!worker.store.isValidPath(path)) continue; auto info = *worker.store.queryPathInfo(path); if (hash.first != info.narHash) { + handleDiffHook(path, actualPath, drvPath); + if (settings.keepFailed) { Path dst = worker.store.toRealPath(path + checkSuffix); deletePath(dst); if (rename(actualPath.c_str(), dst.c_str())) throw SysError(format("renaming '%1%' to '%2%'") % actualPath % dst); + throw Error(format("derivation '%1%' may not be deterministic: output '%2%' differs from '%3%'") % drvPath % path % dst); } else @@ -3254,16 +3269,7 @@ void DerivationGoal::registerOutputs() ? fmt("output '%1%' of '%2%' differs from '%3%' from previous round", i->second.path, drvPath, prev) : fmt("output '%1%' of '%2%' differs from previous round", i->second.path, drvPath); - auto diffHook = settings.diffHook; - if (prevExists && diffHook != "" && runDiffHook) { - try { - auto diff = runProgram(diffHook, true, {prev, i->second.path}); - if (diff != "") - printError(chomp(diff)); - } catch (Error & error) { - printError("diff hook execution failed: %s", error.what()); - } - } + handleDiffHook(prev, i->second.path, drvPath); if (settings.enforceDeterminism) throw NotDeterministic(msg); -- cgit 1.4.1 From 6df61db0600ca73ccd51e3e5bec5312a04e99da1 Mon Sep 17 00:00:00 2001 From: Graham Christensen Date: Fri, 10 May 2019 20:59:39 -0400 Subject: diff hook: execute as the build user, and pass the temp dir --- doc/manual/advanced-topics/diff-hook.xml | 12 ++++------ doc/manual/command-ref/conf-file.xml | 20 ++++++++++------ src/libstore/build.cc | 41 ++++++++++++++++++++++---------- src/libutil/util.cc | 4 ++-- src/libutil/util.hh | 2 ++ 5 files changed, 51 insertions(+), 28 deletions(-) (limited to 'src') diff --git a/doc/manual/advanced-topics/diff-hook.xml b/doc/manual/advanced-topics/diff-hook.xml index d2613f6df227..fb4bf819f94b 100644 --- a/doc/manual/advanced-topics/diff-hook.xml +++ b/doc/manual/advanced-topics/diff-hook.xml @@ -46,17 +46,15 @@ file containing: #!/bin/sh exec >&2 echo "For derivation $3:" -/run/current-system/sw/bin/runuser -u nobody -- /run/current-system/sw/bin/diff -r "$1" "$2" +/run/current-system/sw/bin/diff -r "$1" "$2" - - The diff hook can be run as root. Take care to run as little - as possible as root, for this example we use runuser - to drop privileges. - - +The diff hook is executed by the same user and group who ran the +build. However, the diff hook does not have write access to the store +path just built. +
Spot-Checking Build Determinism diff --git a/doc/manual/command-ref/conf-file.xml b/doc/manual/command-ref/conf-file.xml index a1a5d6e12972..c5f90481b136 100644 --- a/doc/manual/command-ref/conf-file.xml +++ b/doc/manual/command-ref/conf-file.xml @@ -252,13 +252,11 @@ false</literal>.</para> same. </para> - <warning> - <para> - The root user executes the diff hook in a daemonised - installation. See <xref linkend="chap-diff-hook" /> for - information on using the diff hook safely. - </para> - </warning> + <para> + The diff hook is executed by the same user and group who ran the + build. However, the diff hook does not have write access to the + store path just built. + </para> <para>The diff hook program receives three parameters:</para> @@ -280,6 +278,14 @@ false</literal>.</para> The path to the build's derivation </para> </listitem> + + <listitem> + <para> + The path to the build's scratch directory. This directory + will exist only if the build was run with + <option>--keep-failed</option>. + </para> + </listitem> </orderedlist> <para>The diff hook should not print data to stderr or stdout, as diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 026828c535ca..f38d2eaa0cde 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -461,17 +461,26 @@ static void commonChildInit(Pipe & logPipe) close(fdDevNull); } -void handleDiffHook(Path tryA, Path tryB, Path drvPath) +void handleDiffHook(bool allowVfork, uid_t uid, uid_t gid, Path tryA, Path tryB, Path drvPath, Path tmpDir) { auto diffHook = settings.diffHook; if (diffHook != "" && settings.runDiffHook) { - try { - auto diff = runProgram(diffHook, true, {tryA, tryB, drvPath}); - if (diff != "") - printError(chomp(diff)); - } catch (Error & error) { - printError("diff hook execution failed: %s", error.what()); - } + auto wrapper = [&]() { + if (setgid(gid) == -1) + throw SysError("setgid failed"); + if (setuid(uid) == -1) + throw SysError("setuid failed"); + + try { + auto diff = runProgram(diffHook, true, {tryA, tryB, drvPath, tmpDir}); + if (diff != "") + printError(chomp(diff)); + } catch (Error & error) { + printError("diff hook execution failed: %s", error.what()); + } + }; + + doFork(allowVfork, wrapper); } } @@ -3197,14 +3206,18 @@ void DerivationGoal::registerOutputs() if (!worker.store.isValidPath(path)) continue; auto info = *worker.store.queryPathInfo(path); if (hash.first != info.narHash) { - handleDiffHook(path, actualPath, drvPath); - - if (settings.keepFailed) { + if (settings.runDiffHook || settings.keepFailed) { Path dst = worker.store.toRealPath(path + checkSuffix); deletePath(dst); if (rename(actualPath.c_str(), dst.c_str())) throw SysError(format("renaming '%1%' to '%2%'") % actualPath % dst); + handleDiffHook( + !buildUser && !drv->isBuiltin(), + buildUser ? buildUser->getUID() : getuid(), + buildUser ? buildUser->getGID() : getgid(), + path, dst, drvPath, tmpDir); + throw Error(format("derivation '%1%' may not be deterministic: output '%2%' differs from '%3%'") % drvPath % path % dst); } else @@ -3269,7 +3282,11 @@ void DerivationGoal::registerOutputs() ? fmt("output '%1%' of '%2%' differs from '%3%' from previous round", i->second.path, drvPath, prev) : fmt("output '%1%' of '%2%' differs from previous round", i->second.path, drvPath); - handleDiffHook(prev, i->second.path, drvPath); + handleDiffHook( + !buildUser && !drv->isBuiltin(), + buildUser ? buildUser->getUID() : getuid(), + buildUser ? buildUser->getGID() : getgid(), + prev, i->second.path, drvPath, tmpDir); if (settings.enforceDeterminism) throw NotDeterministic(msg); diff --git a/src/libutil/util.cc b/src/libutil/util.cc index a7170566533e..0f4d3d92b866 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -914,8 +914,8 @@ void killUser(uid_t uid) /* Wrapper around vfork to prevent the child process from clobbering the caller's stack frame in the parent. */ -static pid_t doFork(bool allowVfork, std::function<void()> fun) __attribute__((noinline)); -static pid_t doFork(bool allowVfork, std::function<void()> fun) +pid_t doFork(bool allowVfork, std::function<void()> fun) __attribute__((noinline)); +pid_t doFork(bool allowVfork, std::function<void()> fun) { #ifdef __linux__ pid_t pid = allowVfork ? vfork() : fork(); diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 54936a5cb10b..824a35b987e8 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -265,6 +265,8 @@ string runProgram(Path program, bool searchPath = false, const Strings & args = Strings(), const std::optional<std::string> & input = {}); +pid_t doFork(bool allowVfork, std::function<void()> fun); + struct RunOptions { Path program; -- cgit 1.4.1 From dde8eeb39ae9fb73011462c74e5fa6405e432147 Mon Sep 17 00:00:00 2001 From: Graham Christensen <graham@grahamc.com> Date: Sat, 11 May 2019 15:57:38 -0400 Subject: chdir, setgroups --- src/libstore/build.cc | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'src') diff --git a/src/libstore/build.cc b/src/libstore/build.cc index f38d2eaa0cde..8397cd0d1dd7 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -466,8 +466,12 @@ void handleDiffHook(bool allowVfork, uid_t uid, uid_t gid, Path tryA, Path tryB, auto diffHook = settings.diffHook; if (diffHook != "" && settings.runDiffHook) { auto wrapper = [&]() { + if (chdir("/") == -1) + throw SysError("chdir / failed"); if (setgid(gid) == -1) throw SysError("setgid failed"); + if (setgroups(0, 0) == -1) + throw SysError("setgroups failed"); if (setuid(uid) == -1) throw SysError("setuid failed"); -- cgit 1.4.1 From b4a05edbfe49f87555fd284dfb0d6c56ed43217d Mon Sep 17 00:00:00 2001 From: Graham Christensen <graham@grahamc.com> Date: Sat, 11 May 2019 16:35:53 -0400 Subject: runProgram: support gid, uid, chdir --- src/libstore/build.cc | 36 ++++++++++++++++-------------------- src/libutil/util.cc | 15 +++++++++++++-- src/libutil/util.hh | 5 +++-- 3 files changed, 32 insertions(+), 24 deletions(-) (limited to 'src') diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 8397cd0d1dd7..8902e22bd8a7 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -465,26 +465,22 @@ void handleDiffHook(bool allowVfork, uid_t uid, uid_t gid, Path tryA, Path tryB, { auto diffHook = settings.diffHook; if (diffHook != "" && settings.runDiffHook) { - auto wrapper = [&]() { - if (chdir("/") == -1) - throw SysError("chdir / failed"); - if (setgid(gid) == -1) - throw SysError("setgid failed"); - if (setgroups(0, 0) == -1) - throw SysError("setgroups failed"); - if (setuid(uid) == -1) - throw SysError("setuid failed"); - - try { - auto diff = runProgram(diffHook, true, {tryA, tryB, drvPath, tmpDir}); - if (diff != "") - printError(chomp(diff)); - } catch (Error & error) { - printError("diff hook execution failed: %s", error.what()); - } - }; - - doFork(allowVfork, wrapper); + try { + RunOptions diffHookOptions(diffHook,{tryA, tryB, drvPath, tmpDir}); + diffHookOptions.searchPath = true; + diffHookOptions.uid = uid; + diffHookOptions.gid = gid; + diffHookOptions.chdir = "/"; + + auto diffRes = runProgram(diffHookOptions); + if (!statusOk(diffRes.first)) + throw ExecError(diffRes.first, fmt("diff-hook program '%1%' %2%", diffHook, statusToString(diffRes.first))); + + if (diffRes.second != "") + printError(chomp(diffRes.second)); + } catch (Error & error) { + printError("diff hook execution failed: %s", error.what()); + } } } diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 0f4d3d92b866..55b9144f314e 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -16,6 +16,7 @@ #include <future> #include <fcntl.h> +#include <grp.h> #include <limits.h> #include <pwd.h> #include <sys/ioctl.h> @@ -914,8 +915,8 @@ void killUser(uid_t uid) /* Wrapper around vfork to prevent the child process from clobbering the caller's stack frame in the parent. */ -pid_t doFork(bool allowVfork, std::function<void()> fun) __attribute__((noinline)); -pid_t doFork(bool allowVfork, std::function<void()> fun) +static pid_t doFork(bool allowVfork, std::function<void()> fun) __attribute__((noinline)); +static pid_t doFork(bool allowVfork, std::function<void()> fun) { #ifdef __linux__ pid_t pid = allowVfork ? vfork() : fork(); @@ -1025,6 +1026,16 @@ void runProgram2(const RunOptions & options) if (source && dup2(in.readSide.get(), STDIN_FILENO) == -1) throw SysError("dupping stdin"); + //if (options.chdir && chdir((*options.chdir).c_str()) == -1) + // throw SysError("chdir failed"); + if (options.gid && setgid(*options.gid) == -1) + throw SysError("setgid failed"); + /* Drop all other groups if we're setgid. */ + if (options.gid && setgroups(0, 0) == -1) + throw SysError("setgroups failed"); + if (options.uid && setuid(*options.uid) == -1) + throw SysError("setuid failed"); + Strings args_(options.args); args_.push_front(options.program); diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 824a35b987e8..7c57d0afad98 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -265,10 +265,11 @@ string runProgram(Path program, bool searchPath = false, const Strings & args = Strings(), const std::optional<std::string> & input = {}); -pid_t doFork(bool allowVfork, std::function<void()> fun); - struct RunOptions { + std::optional<uid_t> uid; + std::optional<uid_t> gid; + std::optional<Path> chdir; Path program; bool searchPath = true; Strings args; -- cgit 1.4.1 From 73b797c207e1c7a0fd9059d2cf1e3479502f8f1b Mon Sep 17 00:00:00 2001 From: Graham Christensen <graham@grahamc.com> Date: Sun, 12 May 2019 13:44:22 -0400 Subject: handleDiffHook: stop passing allowVfork --- src/libstore/build.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'src') diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 8902e22bd8a7..b07461013cc2 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -461,7 +461,7 @@ static void commonChildInit(Pipe & logPipe) close(fdDevNull); } -void handleDiffHook(bool allowVfork, uid_t uid, uid_t gid, Path tryA, Path tryB, Path drvPath, Path tmpDir) +void handleDiffHook(uid_t uid, uid_t gid, Path tryA, Path tryB, Path drvPath, Path tmpDir) { auto diffHook = settings.diffHook; if (diffHook != "" && settings.runDiffHook) { @@ -3213,7 +3213,6 @@ void DerivationGoal::registerOutputs() throw SysError(format("renaming '%1%' to '%2%'") % actualPath % dst); handleDiffHook( - !buildUser && !drv->isBuiltin(), buildUser ? buildUser->getUID() : getuid(), buildUser ? buildUser->getGID() : getgid(), path, dst, drvPath, tmpDir); @@ -3283,7 +3282,6 @@ void DerivationGoal::registerOutputs() : fmt("output '%1%' of '%2%' differs from previous round", i->second.path, drvPath); handleDiffHook( - !buildUser && !drv->isBuiltin(), buildUser ? buildUser->getUID() : getuid(), buildUser ? buildUser->getGID() : getgid(), prev, i->second.path, drvPath, tmpDir); -- cgit 1.4.1 From f1b8e9efe77014655f059b44afa05c38990dc4aa Mon Sep 17 00:00:00 2001 From: Graham Christensen <graham@grahamc.com> Date: Sun, 12 May 2019 17:03:01 -0400 Subject: runProgram: Uncomment chdir support --- src/libutil/util.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 55b9144f314e..17aee2d5c3d0 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1026,8 +1026,8 @@ void runProgram2(const RunOptions & options) if (source && dup2(in.readSide.get(), STDIN_FILENO) == -1) throw SysError("dupping stdin"); - //if (options.chdir && chdir((*options.chdir).c_str()) == -1) - // throw SysError("chdir failed"); + if (options.chdir && chdir((*options.chdir).c_str()) == -1) + throw SysError("chdir failed"); if (options.gid && setgid(*options.gid) == -1) throw SysError("setgid failed"); /* Drop all other groups if we're setgid. */ -- cgit 1.4.1