From 73acb8b836affe5dfade9dd6e3339ad2f9191add Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 27 Jul 2012 12:16:02 -0400 Subject: [PATCH] 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. --- scripts/copy-from-other-stores.pl.in | 5 +-- scripts/download-from-binary-cache.pl.in | 11 +++---- scripts/download-using-manifests.pl.in | 13 ++------ src/libstore/build.cc | 39 ++++++++++++++++++++---- tests/substituter.sh | 1 + 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 >(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