From 0fa289f559708407ab4384739c0f24258c114b44 Mon Sep 17 00:00:00 2001 From: julia Date: Sat, 18 May 2024 15:38:33 +1000 Subject: [PATCH 1/3] Harmonise the Store::queryPathInfoUncached interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This: - Consistently returns `nullptr` for a non-existent store path, instead of a mix of `nullptr` and throwing exceptions. - If a store returns "bad" store paths in response to a request (e.g. incorrect hash or name), don't cache this result. This removes some duplication of code at the cache-access layer of queryPathInfo() checking this, and ­allows us to provide more specific errors. Part of #270. Change-Id: I86612c6499b1a37ab872c712c2304d6a3ff19edb --- src/libstore/binary-cache-store.cc | 2 +- src/libstore/local-store.cc | 2 +- src/libstore/remote-store.cc | 6 +++--- src/libstore/store-api.cc | 34 ++++++++++++++++++++---------- src/libstore/store-api.hh | 4 ++++ 5 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index ea643fd31..3600eca60 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -361,7 +361,7 @@ std::shared_ptr BinaryCacheStore::queryPathInfoUncached(con auto data = getFile(narInfoFile); - if (!data) return {}; + if (!data) return nullptr; stats.narInfoRead++; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index f5eaa9f5f..bae5fad7b 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -899,7 +899,7 @@ std::shared_ptr LocalStore::queryPathInfoInternal(State & s auto useQueryPathInfo(state.stmts->QueryPathInfo.use()(printStorePath(path))); if (!useQueryPathInfo.next()) - return std::shared_ptr(); + return nullptr; auto id = useQueryPathInfo.getInt(0); diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 93b1afabd..966dd3fe0 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -309,14 +309,14 @@ std::shared_ptr RemoteStore::queryPathInfoUncached(const St try { conn.processStderr(); } catch (Error & e) { - // Ugly backwards compatibility hack. + // Ugly backwards compatibility hack. TODO(fj#325): remove. if (e.msg().find("is not valid") != std::string::npos) - throw InvalidPath(std::move(e.info())); + return nullptr; throw; } if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 17) { bool valid; conn->from >> valid; - if (!valid) throw InvalidPath("path '%s' is not valid", printStorePath(path)); + if (!valid) return nullptr; } return std::make_shared( StorePath{path}, diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 952958d51..e0e842060 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -666,11 +666,19 @@ bool Store::isValidPathUncached(const StorePath & path) } -static bool goodStorePath(const StorePath & expected, const StorePath & actual) +static void ensureGoodStorePath(Store * store, const StorePath & expected, const StorePath & actual) { - return - expected.hashPart() == actual.hashPart() - && (expected.name() == Store::MissingName || expected.name() == actual.name()); + if (expected.hashPart() != actual.hashPart()) { + throw Error( + "the queried store path hash '%s' did not match expected '%s' while querying the store path '%s'", + expected.hashPart(), actual.hashPart(), store->printStorePath(expected) + ); + } else if (expected.name() != Store::MissingName && expected.name() != actual.name()) { + throw Error( + "the queried store path name '%s' did not match expected '%s' while querying the store path '%s'", + expected.name(), actual.name(), store->printStorePath(expected) + ); + } } @@ -683,7 +691,7 @@ ref Store::queryPathInfo(const StorePath & storePath) if (res && res->isKnownNow()) { stats.narInfoReadAverted++; if (!res->didExist()) - throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); + throw InvalidPath("path '%s' does not exist in the store", printStorePath(storePath)); return ref(res->value); } } @@ -696,27 +704,31 @@ ref Store::queryPathInfo(const StorePath & storePath) auto state_(state.lock()); state_->pathInfoCache.upsert(std::string(storePath.to_string()), res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue{ .value = res.second }); - if (res.first == NarInfoDiskCache::oInvalid || - !goodStorePath(storePath, res.second->path)) - throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); + if (res.first == NarInfoDiskCache::oInvalid) + throw InvalidPath("path '%s' does not exist in the store", printStorePath(storePath)); } return ref(res.second); } } auto info = queryPathInfoUncached(storePath); + if (info) { + // first, before we cache anything, check that the store gave us valid data. + ensureGoodStorePath(this, storePath, info->path); + } - if (diskCache) + if (diskCache) { diskCache->upsertNarInfo(getUri(), hashPart, info); + } { auto state_(state.lock()); state_->pathInfoCache.upsert(std::string(storePath.to_string()), PathInfoCacheValue { .value = info }); } - if (!info || !goodStorePath(storePath, info->path)) { + if (!info) { stats.narInfoMissing++; - throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); + throw InvalidPath("path '%s' does not exist in the store", printStorePath(storePath)); } return ref(info); diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 397ebe759..de4dc3f03 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -399,6 +399,10 @@ public: protected: + /** + * Queries the path info without caching. + * Note to implementors: should return `nullptr` when the path is not found. + */ virtual std::shared_ptr queryPathInfoUncached(const StorePath & path) = 0; virtual std::shared_ptr queryRealisationUncached(const DrvOutput &) = 0; From 6c311a4afa339b5dc4f80f03e29c9e7fe779abd5 Mon Sep 17 00:00:00 2001 From: julia Date: Sat, 18 May 2024 18:57:38 +1000 Subject: [PATCH 2/3] Add a clearer error message for InvalidPathError during evaluation Part of #270, #271 Change-Id: I864d7340f26d3c0f9c45db7b6b545face38d8294 --- src/libexpr/eval-error.hh | 6 +++++- src/libstore/store-api.hh | 9 +++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/libexpr/eval-error.hh b/src/libexpr/eval-error.hh index 4f0e0d24c..19540d612 100644 --- a/src/libexpr/eval-error.hh +++ b/src/libexpr/eval-error.hh @@ -47,12 +47,16 @@ MakeError(MissingArgumentError, EvalError); MakeError(RestrictedPathError, Error); MakeError(InfiniteRecursionError, EvalError); +/** + * Represents an exception due to an invalid path; that is, it does not exist. + * It corresponds to `!Store::validPath()`. + */ struct InvalidPathError : public EvalError { public: Path path; InvalidPathError(EvalState & state, const Path & path) - : EvalError(state, "path '%s' is not valid", path) + : EvalError(state, "path '%s' did not exist in the store during evaluation", path) { } }; diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index de4dc3f03..186437f43 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -62,10 +62,18 @@ MakeError(SubstError, Error); * denotes a permanent build failure */ MakeError(BuildError, Error); +/** + * denotes that a path in the store did not exist (but it could, had it + * been put there, i.e. it is still legal). + */ MakeError(InvalidPath, Error); MakeError(Unsupported, Error); MakeError(SubstituteGone, Error); MakeError(SubstituterDisabled, Error); +/** + * denotes that a path could not possibly be a store path. + * e.g. outside of the nix store, illegal characters in the name, etc. +*/ MakeError(BadStorePath, Error); MakeError(InvalidStoreURI, Error); @@ -328,6 +336,7 @@ public: /** * Check whether a path is valid. + * A path is valid when it exists in the store *now*. */ bool isValidPath(const StorePath & path); From 89c782b0c0df6ca9d85207b62318e70729f18e24 Mon Sep 17 00:00:00 2001 From: julia Date: Sat, 18 May 2024 20:16:32 +1000 Subject: [PATCH 3/3] Change error messages about 'invalid paths' to 'path does not exist'. Fixes #270. Change-Id: I07d2da41498cfdf324a03af40533044d58c97c7e --- src/libexpr/primops.cc | 3 ++- src/libstore/binary-cache-store.cc | 2 +- src/libstore/local-fs-store.cc | 4 ++-- src/libstore/local-store.cc | 6 +++--- src/libstore/remote-fs-accessor.cc | 2 +- 5 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 3cc2659fb..dba56c011 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -383,7 +383,8 @@ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v) try { auto _ = state.realiseContext(context); // FIXME: Handle CA derivations } catch (InvalidPathError & e) { - state.error("cannot execute '%1%', since path '%2%' is not valid", program, e.path).atPos(pos).debugThrow(); + e.addTrace(state.positions[pos], "while realising the context for builtins.exec"); + throw; } auto output = runProgram(program, true, commandArgs); diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index 3600eca60..7df55a32d 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -170,7 +170,7 @@ ref BinaryCacheStore::addToStoreCommon( if (ref != info.path) queryPathInfo(ref); } catch (InvalidPath &) { - throw Error("cannot add '%s' to the binary cache because the reference '%s' is not valid", + throw Error("cannot add '%s' to the binary cache because the reference '%s' does not exist", printStorePath(info.path), printStorePath(ref)); } diff --git a/src/libstore/local-fs-store.cc b/src/libstore/local-fs-store.cc index b224fc3e9..56f13920c 100644 --- a/src/libstore/local-fs-store.cc +++ b/src/libstore/local-fs-store.cc @@ -23,7 +23,7 @@ struct LocalStoreAccessor : public FSAccessor { auto storePath = store->toStorePath(path).first; if (requireValidPath && !store->isValidPath(storePath)) - throw InvalidPath("path '%1%' is not a valid store path", store->printStorePath(storePath)); + throw InvalidPath("path '%1%' does not exist in the store", store->printStorePath(storePath)); return store->getRealStoreDir() + std::string(path, store->storeDir.size()); } @@ -81,7 +81,7 @@ ref LocalFSStore::getFSAccessor() void LocalFSStore::narFromPath(const StorePath & path, Sink & sink) { if (!isValidPath(path)) - throw Error("path '%s' is not valid", printStorePath(path)); + throw Error("path '%s' does not exist in store", printStorePath(path)); dumpPath(getRealStoreDir() + std::string(printStorePath(path), storeDir.size()), sink); } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index bae5fad7b..5bdf0362d 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -907,7 +907,7 @@ std::shared_ptr LocalStore::queryPathInfoInternal(State & s try { narHash = Hash::parseAnyPrefixed(useQueryPathInfo.getStr(1)); } catch (BadHash & e) { - throw Error("invalid-path entry for '%s': %s", printStorePath(path), e.what()); + throw BadStorePath("bad hash in store path '%s': %s", printStorePath(path), e.what()); } auto info = std::make_shared(path, narHash); @@ -957,8 +957,8 @@ void LocalStore::updatePathInfo(State & state, const ValidPathInfo & info) uint64_t LocalStore::queryValidPathId(State & state, const StorePath & path) { auto use(state.stmts->QueryPathInfo.use()(printStorePath(path))); - if (!use.next()) - throw InvalidPath("path '%s' is not valid", printStorePath(path)); + if (!use.next()) // TODO: I guess if SQLITE got corrupted..? + throw InvalidPath("path '%s' does not exist", printStorePath(path)); return use.getInt(0); } diff --git a/src/libstore/remote-fs-accessor.cc b/src/libstore/remote-fs-accessor.cc index c8e20b3b5..8da8b9774 100644 --- a/src/libstore/remote-fs-accessor.cc +++ b/src/libstore/remote-fs-accessor.cc @@ -55,7 +55,7 @@ std::pair, Path> RemoteFSAccessor::fetch(const Path & path_, boo auto [storePath, restPath] = store->toStorePath(path); if (requireValidPath && !store->isValidPath(storePath)) - throw InvalidPath("path '%1%' is not a valid store path", store->printStorePath(storePath)); + throw InvalidPath("path '%1%' does not exist in remote store", store->printStorePath(storePath)); auto i = nars.find(std::string(storePath.hashPart())); if (i != nars.end()) return {i->second, restPath};