Let build.cc verify the expected hash of a substituter's output

Since SubstitutionGoal::finished() in build.cc computes the hash
anyway, we can prevent the inefficiency of computing the hash twice by
letting the substituter tell Nix about the expected hash, which can
then verify it.

5 files changed, 43 insertions, 26 deletions

diff --git a/scripts/copy-from-other-stores.pl.in b/scripts/copy-from-other-stores.pl.in
index 92869ee7a..3ee6f075b 100755
--- a/scripts/copy-from-other-stores.pl.in
+++ b/scripts/copy-from-other-stores.pl.in
@@ -52,7 +52,7 @@ if ($ARGV[0] eq "--query") {
                 next unless defined $store;
 
                 $ENV{"NIX_DB_DIR"} = "$store/var/nix/db";
-            
+
                 my $deriver = `@bindir@/nix-store --query --deriver $storePath`;
                 die "cannot query deriver of `$storePath'" if $? != 0;
                 chomp $deriver;
@@ -87,9 +87,10 @@ elsif ($ARGV[0] eq "--substitute") {
     my $storePath = $ARGV[1];
     my ($store, $sourcePath) = findStorePath $storePath;
     die unless $store;
-    print "\n*** Copying `$storePath' from `$sourcePath'\n\n";
+    print STDERR "\n*** Copying `$storePath' from `$sourcePath'\n\n";
     system("$binDir/nix-store --dump $sourcePath | $binDir/nix-store --restore $storePath") == 0
         or die "cannot copy `$sourcePath' to `$storePath'";
+    print "\n"; # no hash to verify
 }
 
 
diff --git a/scripts/download-from-binary-cache.pl.in b/scripts/download-from-binary-cache.pl.in
index 9e1c774a5..823ecd9d9 100644
--- a/scripts/download-from-binary-cache.pl.in
+++ b/scripts/download-from-binary-cache.pl.in
@@ -432,13 +432,10 @@ sub downloadBinary {
             die "download of `$info->{url}' failed" . ($! ? ": $!" : "") . "\n" unless $? == 0;
             next;
         }
-        # The hash in the manifest can be either in base-16 or
-        # base-32.  Handle both.
-        $info->{narHash} =~ /^sha256:(.*)$/ or die "invalid hash";
-        my $hash = $1;
-        my $hash2 = hashPath("sha256", 1, $storePath);
-        die "hash mismatch in downloaded path ‘$storePath’; expected $hash, got $hash2\n"
-            if $hash ne $hash2;
+
+        # Tell Nix about the expected hash so it can verify it.
+        print "$info->{narHash}\n";
+
         print STDERR "\n";
         return 1;
     }
diff --git a/scripts/download-using-manifests.pl.in b/scripts/download-using-manifests.pl.in
index ed63e792e..04bcce90d 100755
--- a/scripts/download-using-manifests.pl.in
+++ b/scripts/download-using-manifests.pl.in
@@ -353,19 +353,10 @@ while (scalar @path > 0) {
 }
 
 
-# Make sure that the hash declared in the manifest matches what we
-# downloaded and unpacked.
+# Tell Nix about the expected hash so it can verify it.
 die "cannot check integrity of the downloaded path since its hash is not known\n"
     unless defined $finalNarHash;
-
-my ($hashAlgo, $hash) = parseHash $finalNarHash;
-
-# The hash in the manifest can be either in base-16 or base-32.
-# Handle both.
-my $hash2 = hashPath($hashAlgo, $hashAlgo eq "sha256" && length($hash) != 64, $targetPath);
-
-die "hash mismatch in downloaded path $targetPath; expected $hash, got $hash2\n"
-    if $hash ne $hash2;
+print "$finalNarHash\n";
 
 
 print STDERR "\n";
diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index 90dc2b79b..887858fce 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -371,6 +371,7 @@ void commonChildInit(Pipe & logPipe)
     if (dup2(logPipe.writeSide, STDERR_FILENO) == -1)
         throw SysError("cannot pipe standard error into log file");
     logPipe.readSide.close();
+    logPipe.writeSide.close();
 
     /* Dup stderr to stdout. */
     if (dup2(STDERR_FILENO, STDOUT_FILENO) == -1)
@@ -2273,7 +2274,10 @@ private:
     /* Path info returned by the substituter's query info operation. */
     SubstitutablePathInfo info;
 
-    /* Pipe for the substitute's standard output/error. */
+    /* Pipe for the substituter's standard output. */
+    Pipe outPipe;
+
+    /* Pipe for the substituter's standard error. */
     Pipe logPipe;
 
     /* The process ID of the builder. */
@@ -2466,6 +2470,7 @@ void SubstitutionGoal::tryToRun()
 
     printMsg(lvlInfo, format("fetching path `%1%'...") % storePath);
 
+    outPipe.create();
     logPipe.create();
 
     /* Remove the (stale) output path if it exists. */
@@ -2482,10 +2487,13 @@ void SubstitutionGoal::tryToRun()
     case 0:
         try { /* child */
 
-            logPipe.readSide.close();
-
             commonChildInit(logPipe);
 
+            if (dup2(outPipe.writeSide, STDOUT_FILENO) == -1)
+                throw SysError("cannot dup output pipe into stdout");
+            outPipe.readSide.close();
+            outPipe.writeSide.close();
+
             /* Fill in the arguments. */
             Strings args;
             args.push_back(baseNameOf(sub));
@@ -2506,6 +2514,7 @@ void SubstitutionGoal::tryToRun()
     /* parent */
     pid.setSeparatePG(true);
     pid.setKillSignal(SIGTERM);
+    outPipe.writeSide.close();
     logPipe.writeSide.close();
     worker.childStarted(shared_from_this(),
         pid, singleton<set<int> >(logPipe.readSide), true, true);
@@ -2534,9 +2543,12 @@ void SubstitutionGoal::finished()
     /* Close the read side of the logger pipe. */
     logPipe.readSide.close();
 
-    debug(format("substitute for `%1%' finished") % storePath);
+    /* Get the hash info from stdout. */
+    string expectedHashStr = statusOk(status) ? readLine(outPipe.readSide) : "";
+    outPipe.readSide.close();
 
     /* Check the exit status and the build result. */
+    HashResult hash;
     try {
 
         if (!statusOk(status))
@@ -2546,6 +2558,23 @@ void SubstitutionGoal::finished()
         if (!pathExists(storePath))
             throw SubstError(format("substitute did not produce path `%1%'") % storePath);
 
+        hash = hashPath(htSHA256, storePath);
+
+        /* Verify the expected hash we got from the substituer. */
+        if (expectedHashStr != "") {
+            size_t n = expectedHashStr.find(':');
+            if (n == string::npos)
+                throw Error(format("bad hash from substituter: %1%") % expectedHashStr);
+            HashType hashType = parseHashType(string(expectedHashStr, 0, n));
+            if (hashType == htUnknown)
+                throw Error(format("unknown hash algorithm in `%1%'") % expectedHashStr);
+            Hash expectedHash = parseHash16or32(hashType, string(expectedHashStr, n + 1));
+            Hash actualHash = hashType == htSHA256 ? hash.first : hashPath(hashType, storePath).first;
+            if (expectedHash != actualHash)
+                throw SubstError(format("hash mismatch in downloaded path `%1%': expected %2%, got %3%")
+                    % storePath % printHash(expectedHash) % printHash(actualHash));
+        }
+
     } catch (SubstError & e) {
 
         printMsg(lvlInfo, e.msg());
@@ -2563,8 +2592,6 @@ void SubstitutionGoal::finished()
 
     canonicalisePathMetaData(storePath);
 
-    HashResult hash = hashPath(htSHA256, storePath);
-
     worker.store.optimisePath(storePath); // FIXME: combine with hashPath()
 
     ValidPathInfo info2;
diff --git a/tests/substituter.sh b/tests/substituter.sh
index a6bdacfd6..885655760 100755
--- a/tests/substituter.sh
+++ b/tests/substituter.sh
@@ -29,6 +29,7 @@ if test $1 = "--query"; then
 elif test $1 = "--substitute"; then
     mkdir $2
     echo "Hallo Wereld" > $2/hello
+    echo # no expected hash
 else
     echo "unknown substituter operation"
     exit 1