From 450dcf2c1b60a36f5ffeab2411805287d122bcdd Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 2 Jun 2020 15:52:13 +0000 Subject: [PATCH 1/6] Remove `HashType::Unknown` Instead, `Hash` uses `std::optional`. In the future, we may also make `Hash` itself require a known hash type, encoraging people to use `std::optional` instead. --- src/libexpr/primops.cc | 16 +++--- src/libstore/build.cc | 6 +-- src/libstore/builtins/fetchurl.cc | 2 +- src/libstore/derivations.cc | 2 - src/libstore/export-import.cc | 2 +- src/libstore/local-store.cc | 4 +- src/libutil/args.cc | 2 - src/libutil/hash.cc | 62 ++++++++++++++++-------- src/libutil/hash.hh | 15 ++++-- src/libutil/tests/hash.cc | 2 +- src/nix-prefetch-url/nix-prefetch-url.cc | 2 - src/nix-store/nix-store.cc | 2 +- src/nix/hash.cc | 4 +- src/nix/verify.cc | 4 +- 14 files changed, 72 insertions(+), 53 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 8fbcef8c8..63bc949a6 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -718,7 +718,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * if (outputs.size() != 1 || *(outputs.begin()) != "out") throw Error(format("multiple outputs are not supported in fixed-output derivations, at %1%") % posDrvName); - HashType ht = outputHashAlgo.empty() ? HashType::Unknown : parseHashType(outputHashAlgo); + std::optional ht = parseHashTypeOpt(outputHashAlgo); Hash h(*outputHash, ht); auto outPath = state.store->makeFixedOutputPath(ingestionMethod, h, drvName); @@ -726,7 +726,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * drv.outputs.insert_or_assign("out", DerivationOutput { std::move(outPath), (ingestionMethod == FileIngestionMethod::Recursive ? "r:" : "") - + printHashType(h.type), + + printHashType(*h.type), h.to_string(Base::Base16, false), }); } @@ -934,14 +934,14 @@ static void prim_findFile(EvalState & state, const Pos & pos, Value * * args, Va static void prim_hashFile(EvalState & state, const Pos & pos, Value * * args, Value & v) { string type = state.forceStringNoCtx(*args[0], pos); - HashType ht = parseHashType(type); - if (ht == HashType::Unknown) + std::optional ht = parseHashType(type); + if (!ht) throw Error(format("unknown hash type '%1%', at %2%") % type % pos); PathSet context; // discarded Path p = state.coerceToPath(pos, *args[1], context); - mkString(v, hashFile(ht, state.checkSourcePath(p)).to_string(Base::Base16, false), context); + mkString(v, hashFile(*ht, state.checkSourcePath(p)).to_string(Base::Base16, false), context); } /* Read a directory (without . or ..) */ @@ -1812,14 +1812,14 @@ static void prim_stringLength(EvalState & state, const Pos & pos, Value * * args static void prim_hashString(EvalState & state, const Pos & pos, Value * * args, Value & v) { string type = state.forceStringNoCtx(*args[0], pos); - HashType ht = parseHashType(type); - if (ht == HashType::Unknown) + std::optional ht = parseHashType(type); + if (!ht) throw Error(format("unknown hash type '%1%', at %2%") % type % pos); PathSet context; // discarded string s = state.forceString(*args[1], context, pos); - mkString(v, hashString(ht, s).to_string(Base::Base16, false), context); + mkString(v, hashString(*ht, s).to_string(Base::Base16, false), context); } diff --git a/src/libstore/build.cc b/src/libstore/build.cc index ae7ba6549..b93855f79 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -3726,8 +3726,8 @@ void DerivationGoal::registerOutputs() /* Check the hash. In hash mode, move the path produced by the derivation to its content-addressed location. */ Hash h2 = outputHashMode == FileIngestionMethod::Recursive - ? hashPath(h.type, actualPath).first - : hashFile(h.type, actualPath); + ? hashPath(*h.type, actualPath).first + : hashFile(*h.type, actualPath); auto dest = worker.store.makeFixedOutputPath(outputHashMode, h2, i.second.path.name()); @@ -4999,7 +4999,7 @@ bool Worker::pathContentsGood(const StorePath & path) if (!pathExists(store.printStorePath(path))) res = false; else { - HashResult current = hashPath(info->narHash.type, store.printStorePath(path)); + HashResult current = hashPath(*info->narHash.type, store.printStorePath(path)); Hash nullHash(HashType::SHA256); res = info->narHash == nullHash || info->narHash == current.first; } diff --git a/src/libstore/builtins/fetchurl.cc b/src/libstore/builtins/fetchurl.cc index b70e960f8..831431437 100644 --- a/src/libstore/builtins/fetchurl.cc +++ b/src/libstore/builtins/fetchurl.cc @@ -65,7 +65,7 @@ void builtinFetchurl(const BasicDerivation & drv, const std::string & netrcData) if (!hasSuffix(hashedMirror, "/")) hashedMirror += '/'; auto ht = parseHashType(getAttr("outputHashAlgo")); auto h = Hash(getAttr("outputHash"), ht); - fetch(hashedMirror + printHashType(h.type) + "/" + h.to_string(Base::Base16, false)); + fetch(hashedMirror + printHashType(*h.type) + "/" + h.to_string(Base::Base16, false)); return; } catch (Error & e) { debug(e.what()); diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index a90c9b86c..d7b677185 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -20,8 +20,6 @@ void DerivationOutput::parseHashInfo(FileIngestionMethod & recursive, Hash & has } HashType hashType = parseHashType(algo); - if (hashType == HashType::Unknown) - throw Error("unknown hash algorithm '%s'", algo); hash = Hash(this->hash, hashType); } diff --git a/src/libstore/export-import.cc b/src/libstore/export-import.cc index 8a5e9d08e..e96b5610a 100644 --- a/src/libstore/export-import.cc +++ b/src/libstore/export-import.cc @@ -54,7 +54,7 @@ void Store::exportPath(const StorePath & path, Sink & sink) filesystem corruption from spreading to other machines. Don't complain if the stored hash is zero (unknown). */ Hash hash = hashAndWriteSink.currentHash(); - if (hash != info->narHash && info->narHash != Hash(info->narHash.type)) + if (hash != info->narHash && info->narHash != Hash(*info->narHash.type)) throw Error("hash of path '%s' has changed from '%s' to '%s'!", printStorePath(path), info->narHash.to_string(), hash.to_string()); diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 7f0d5af25..5b18ebf95 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1264,9 +1264,9 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) std::unique_ptr hashSink; if (info->ca == "" || !info->references.count(info->path)) - hashSink = std::make_unique(info->narHash.type); + hashSink = std::make_unique(*info->narHash.type); else - hashSink = std::make_unique(info->narHash.type, storePathToHash(printStorePath(info->path))); + hashSink = std::make_unique(*info->narHash.type, storePathToHash(printStorePath(info->path))); dumpPath(Store::toRealPath(i), *hashSink); auto current = hashSink->finish(); diff --git a/src/libutil/args.cc b/src/libutil/args.cc index 4a3f5aae8..c4035ab85 100644 --- a/src/libutil/args.cc +++ b/src/libutil/args.cc @@ -162,8 +162,6 @@ Args::Flag Args::Flag::mkHashTypeFlag(std::string && longName, HashType * ht) .labels = {"hash-algo"}, .handler = {[ht](std::string s) { *ht = parseHashType(s); - if (*ht == HashType::Unknown) - throw UsageError("unknown hash type '%1%'", s); }} }; } diff --git a/src/libutil/hash.cc b/src/libutil/hash.cc index 5e6edeec3..0c3de2fda 100644 --- a/src/libutil/hash.cc +++ b/src/libutil/hash.cc @@ -4,6 +4,7 @@ #include #include +#include "args.hh" #include "hash.hh" #include "archive.hh" #include "util.hh" @@ -18,11 +19,13 @@ namespace nix { void Hash::init() { - if (type == HashType::MD5) hashSize = md5HashSize; - else if (type == HashType::SHA1) hashSize = sha1HashSize; - else if (type == HashType::SHA256) hashSize = sha256HashSize; - else if (type == HashType::SHA512) hashSize = sha512HashSize; - else abort(); + if (!type) abort(); + switch (*type) { + case HashType::MD5: hashSize = md5HashSize; break; + case HashType::SHA1: hashSize = sha1HashSize; break; + case HashType::SHA256: hashSize = sha256HashSize; break; + case HashType::SHA512: hashSize = sha512HashSize; break; + } assert(hashSize <= maxHashSize); memset(hash, 0, maxHashSize); } @@ -102,11 +105,18 @@ string printHash16or32(const Hash & hash) } +HashType assertInitHashType(const Hash & h) { + if (h.type) + return *h.type; + else + abort(); +} + std::string Hash::to_string(Base base, bool includeType) const { std::string s; if (base == Base::SRI || includeType) { - s += printHashType(type); + s += printHashType(assertInitHashType(*this)); s += base == Base::SRI ? '-' : ':'; } switch (base) { @@ -124,8 +134,10 @@ std::string Hash::to_string(Base base, bool includeType) const return s; } +Hash::Hash(const std::string & s, HashType type) : Hash(s, std::optional { type }) { } +Hash::Hash(const std::string & s) : Hash(s, std::optional{}) { } -Hash::Hash(const std::string & s, HashType type) +Hash::Hash(const std::string & s, std::optional type) : type(type) { size_t pos = 0; @@ -136,17 +148,17 @@ Hash::Hash(const std::string & s, HashType type) sep = s.find('-'); if (sep != string::npos) { isSRI = true; - } else if (type == HashType::Unknown) + } else if (! type) throw BadHash("hash '%s' does not include a type", s); } if (sep != string::npos) { string hts = string(s, 0, sep); this->type = parseHashType(hts); - if (this->type == HashType::Unknown) + if (!this->type) throw BadHash("unknown hash type '%s'", hts); - if (type != HashType::Unknown && type != this->type) - throw BadHash("hash '%s' should have type '%s'", s, printHashType(type)); + if (type && type != this->type) + throw BadHash("hash '%s' should have type '%s'", s, printHashType(*type)); pos = sep + 1; } @@ -202,7 +214,7 @@ Hash::Hash(const std::string & s, HashType type) } else - throw BadHash("hash '%s' has wrong length for hash type '%s'", s, printHashType(type)); + throw BadHash("hash '%s' has wrong length for hash type '%s'", s, printHashType(*type)); } @@ -318,24 +330,34 @@ Hash compressHash(const Hash & hash, unsigned int newSize) } -HashType parseHashType(const string & s) +std::optional parseHashTypeOpt(const string & s) { if (s == "md5") return HashType::MD5; else if (s == "sha1") return HashType::SHA1; else if (s == "sha256") return HashType::SHA256; else if (s == "sha512") return HashType::SHA512; - else return HashType::Unknown; + else return std::optional {}; } +HashType parseHashType(const string & s) +{ + auto opt_h = parseHashTypeOpt(s); + if (opt_h) + return *opt_h; + else + throw UsageError("unknown hash algorithm '%1%'", s); +} string printHashType(HashType ht) { - if (ht == HashType::MD5) return "md5"; - else if (ht == HashType::SHA1) return "sha1"; - else if (ht == HashType::SHA256) return "sha256"; - else if (ht == HashType::SHA512) return "sha512"; - else abort(); + string ret; + switch (ht) { + case HashType::MD5: ret = "md5"; break; + case HashType::SHA1: ret = "sha1"; break; + case HashType::SHA256: ret = "sha256"; break; + case HashType::SHA512: ret = "sha512"; break; + } + return ret; } - } diff --git a/src/libutil/hash.hh b/src/libutil/hash.hh index 0fe6e7677..41322be67 100644 --- a/src/libutil/hash.hh +++ b/src/libutil/hash.hh @@ -11,7 +11,6 @@ MakeError(BadHash, Error); enum struct HashType : char { - Unknown, MD5, SHA1, SHA256, @@ -40,7 +39,7 @@ struct Hash unsigned int hashSize = 0; unsigned char hash[maxHashSize] = {}; - HashType type = HashType::Unknown; + std::optional type = {}; /* Create an unset hash object. */ Hash() { }; @@ -51,14 +50,18 @@ struct Hash /* Initialize the hash from a string representation, in the format "[:]" or "-" (a Subresource Integrity hash expression). If the 'type' argument - is HashType::Unknown, then the hash type must be specified in the + is not present, then the hash type must be specified in the string. */ - Hash(const std::string & s, HashType type = HashType::Unknown); + Hash(const std::string & s, std::optional type); + // type must be provided + Hash(const std::string & s, HashType type); + // hash type must be part of string + Hash(const std::string & s); void init(); /* Check whether a hash is set. */ - operator bool () const { return type != HashType::Unknown; } + operator bool () const { return (bool) type; } /* Check whether two hash are equal. */ bool operator == (const Hash & h2) const; @@ -127,6 +130,8 @@ Hash compressHash(const Hash & hash, unsigned int newSize); /* Parse a string representing a hash type. */ HashType parseHashType(const string & s); +/* Will return nothing on parse error */ +std::optional parseHashTypeOpt(const string & s); /* And the reverse. */ string printHashType(HashType ht); diff --git a/src/libutil/tests/hash.cc b/src/libutil/tests/hash.cc index c513ce4ac..5fd168be5 100644 --- a/src/libutil/tests/hash.cc +++ b/src/libutil/tests/hash.cc @@ -75,6 +75,6 @@ namespace nix { TEST(hashString, hashingWithUnknownAlgoExits) { auto s = "unknown"; - ASSERT_DEATH(hashString(HashType::Unknown, s), ""); + ASSERT_DEATH(hashString(HashType::SHA512, s), ""); } } diff --git a/src/nix-prefetch-url/nix-prefetch-url.cc b/src/nix-prefetch-url/nix-prefetch-url.cc index 29c32b39b..e60c6615d 100644 --- a/src/nix-prefetch-url/nix-prefetch-url.cc +++ b/src/nix-prefetch-url/nix-prefetch-url.cc @@ -72,8 +72,6 @@ static int _main(int argc, char * * argv) else if (*arg == "--type") { string s = getArg(*arg, arg, end); ht = parseHashType(s); - if (ht == HashType::Unknown) - throw UsageError(format("unknown hash type '%1%'") % s); } else if (*arg == "--print-path") printPath = true; diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index ace593cde..ce86a80b7 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -722,7 +722,7 @@ static void opVerifyPath(Strings opFlags, Strings opArgs) auto path = store->followLinksToStorePath(i); printMsg(Verbosity::Talkative, "checking path '%s'...", store->printStorePath(path)); auto info = store->queryPathInfo(path); - HashSink sink(info->narHash.type); + HashSink sink(*info->narHash.type); store->narFromPath(path, sink); auto current = sink.finish(); if (current.first != info->narHash) { diff --git a/src/nix/hash.cc b/src/nix/hash.cc index 3362ffd0d..4980cd198 100644 --- a/src/nix/hash.cc +++ b/src/nix/hash.cc @@ -79,7 +79,7 @@ static RegisterCommand r2("hash-path", [](){ return make_ref(FileIngest struct CmdToBase : Command { Base base; - HashType ht = HashType::Unknown; + HashType ht; std::vector args; CmdToBase(Base base) : base(base) @@ -132,8 +132,6 @@ static int compatNixHash(int argc, char * * argv) else if (*arg == "--type") { string s = getArg(*arg, arg, end); ht = parseHashType(s); - if (ht == HashType::Unknown) - throw UsageError(format("unknown hash type '%1%'") % s); } else if (*arg == "--to-base16") op = opTo16; else if (*arg == "--to-base32") op = opTo32; diff --git a/src/nix/verify.cc b/src/nix/verify.cc index 0c3478ff5..fa05e7353 100644 --- a/src/nix/verify.cc +++ b/src/nix/verify.cc @@ -88,9 +88,9 @@ struct CmdVerify : StorePathsCommand std::unique_ptr hashSink; if (info->ca == "") - hashSink = std::make_unique(info->narHash.type); + hashSink = std::make_unique(*info->narHash.type); else - hashSink = std::make_unique(info->narHash.type, storePathToHash(store->printStorePath(info->path))); + hashSink = std::make_unique(*info->narHash.type, storePathToHash(store->printStorePath(info->path))); store->narFromPath(info->path, *hashSink); From d73dbc8e4cfe7ad92f072f9ccc30e51df9a5e97b Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 2 Jun 2020 16:28:54 +0000 Subject: [PATCH 2/6] Remove `hashingWithUnknownAlgoExits` A valid hash type must be provided now. The hash itself can still be invalid, but that doesn't cause an `abort()`. --- src/libutil/tests/hash.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/libutil/tests/hash.cc b/src/libutil/tests/hash.cc index 5fd168be5..ecc0d4a03 100644 --- a/src/libutil/tests/hash.cc +++ b/src/libutil/tests/hash.cc @@ -72,9 +72,4 @@ namespace nix { "7299aeadb6889018501d289e4900f7e4331b99dec4b5433a" "c7d329eeb6dd26545e96e55b874be909"); } - - TEST(hashString, hashingWithUnknownAlgoExits) { - auto s = "unknown"; - ASSERT_DEATH(hashString(HashType::SHA512, s), ""); - } } From c502119fd3b9673e966d5c34ec42cbe18baa17b9 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 2 Jun 2020 18:05:26 +0000 Subject: [PATCH 3/6] to-base supports parsing SRI hashes, so make type flag optional --- src/nix/hash.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nix/hash.cc b/src/nix/hash.cc index 4980cd198..0e24bbaed 100644 --- a/src/nix/hash.cc +++ b/src/nix/hash.cc @@ -79,12 +79,12 @@ static RegisterCommand r2("hash-path", [](){ return make_ref(FileIngest struct CmdToBase : Command { Base base; - HashType ht; + std::optional ht; std::vector args; CmdToBase(Base base) : base(base) { - addFlag(Flag::mkHashTypeFlag("type", &ht)); + addFlag(Flag::mkHashTypeFlag("type", &*ht)); expectArgs("strings", &args); } From c664e68b87a3e9e41c4471276886da71793b2d85 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 2 Jun 2020 18:25:32 +0000 Subject: [PATCH 4/6] Fix to-base --type handler to correctly set std::optional flag Now that we have a separate flag function, also describe why it is optional. --- src/libutil/args.cc | 12 ++++++++++++ src/libutil/args.hh | 1 + src/nix/hash.cc | 2 +- 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/libutil/args.cc b/src/libutil/args.cc index c4035ab85..4fe9539e4 100644 --- a/src/libutil/args.cc +++ b/src/libutil/args.cc @@ -166,6 +166,18 @@ Args::Flag Args::Flag::mkHashTypeFlag(std::string && longName, HashType * ht) }; } +Args::Flag Args::Flag::mkHashTypeOptFlag(std::string && longName, std::optional * oht) +{ + return Flag { + .longName = std::move(longName), + .description = "hash algorithm ('md5', 'sha1', 'sha256', or 'sha512'). Optional as can also be gotten from SRI hash itself.", + .labels = {"hash-algo"}, + .handler = {[oht](std::string s) { + *oht = std::optional { parseHashType(s) }; + }} + }; +} + Strings argvToStrings(int argc, char * * argv) { Strings args; diff --git a/src/libutil/args.hh b/src/libutil/args.hh index fc8f82af5..f2315f67a 100644 --- a/src/libutil/args.hh +++ b/src/libutil/args.hh @@ -85,6 +85,7 @@ protected: Handler handler; static Flag mkHashTypeFlag(std::string && longName, HashType * ht); + static Flag mkHashTypeOptFlag(std::string && longName, std::optional * oht); }; std::map longFlags; diff --git a/src/nix/hash.cc b/src/nix/hash.cc index 0e24bbaed..d1b5cca72 100644 --- a/src/nix/hash.cc +++ b/src/nix/hash.cc @@ -84,7 +84,7 @@ struct CmdToBase : Command CmdToBase(Base base) : base(base) { - addFlag(Flag::mkHashTypeFlag("type", &*ht)); + addFlag(Flag::mkHashTypeOptFlag("type", &ht)); expectArgs("strings", &args); } From 1fcd3afc38ebc7ee98add0bf1b4bd643b25ccebf Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 2 Jun 2020 20:35:17 +0000 Subject: [PATCH 5/6] Fix hashes --- src/libutil/hash.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libutil/hash.cc b/src/libutil/hash.cc index 0c3de2fda..6b9effdd2 100644 --- a/src/libutil/hash.cc +++ b/src/libutil/hash.cc @@ -19,13 +19,13 @@ namespace nix { void Hash::init() { - if (!type) abort(); - switch (*type) { + if (!type) abort(); + switch (*type) { case HashType::MD5: hashSize = md5HashSize; break; case HashType::SHA1: hashSize = sha1HashSize; break; case HashType::SHA256: hashSize = sha256HashSize; break; case HashType::SHA512: hashSize = sha512HashSize; break; - } + } assert(hashSize <= maxHashSize); memset(hash, 0, maxHashSize); } From 406dbb7fce32f7d80b02f560d91c956698b58d6e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 2 Jun 2020 21:09:15 +0000 Subject: [PATCH 6/6] `outputHashAlgo` can be blank so parse accordingly It is blank for SRI hashes. --- src/libstore/builtins/fetchurl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/builtins/fetchurl.cc b/src/libstore/builtins/fetchurl.cc index 831431437..770df2927 100644 --- a/src/libstore/builtins/fetchurl.cc +++ b/src/libstore/builtins/fetchurl.cc @@ -63,7 +63,7 @@ void builtinFetchurl(const BasicDerivation & drv, const std::string & netrcData) for (auto hashedMirror : settings.hashedMirrors.get()) try { if (!hasSuffix(hashedMirror, "/")) hashedMirror += '/'; - auto ht = parseHashType(getAttr("outputHashAlgo")); + auto ht = parseHashTypeOpt(getAttr("outputHashAlgo")); auto h = Hash(getAttr("outputHash"), ht); fetch(hashedMirror + printHashType(*h.type) + "/" + h.to_string(Base::Base16, false)); return;