From 22a754c091f765061f59bef5ce091268493bb138 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 28 Feb 2020 18:07:10 +0100 Subject: [PATCH] Fix GC failures on bad store path names It failed on names like '/nix/store/9ip48nkc9rfy0a4yaw98lp6gipqlib1a-'. --- src/libstore/gc.cc | 40 +++++++++++++++++++-------------------- src/libstore/path.cc | 14 ++++++++++++++ src/libstore/store-api.cc | 8 -------- src/libstore/store-api.hh | 2 ++ 4 files changed, 35 insertions(+), 29 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 690febc5b..0c3d89611 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -255,12 +255,11 @@ void LocalStore::findTempRoots(FDs & fds, Roots & tempRoots, bool censor) void LocalStore::findRoots(const Path & path, unsigned char type, Roots & roots) { auto foundRoot = [&](const Path & path, const Path & target) { - Path storePath = toStorePath(target); - // FIXME - if (isStorePath(storePath) && isValidPath(parseStorePath(storePath))) - roots[parseStorePath(storePath)].emplace(path); + auto storePath = maybeParseStorePath(toStorePath(target)); + if (storePath && isValidPath(*storePath)) + roots[std::move(*storePath)].emplace(path); else - printInfo("skipping invalid root from '%1%' to '%2%'", path, storePath); + printInfo("skipping invalid root from '%1%' to '%2%'", path, target); }; try { @@ -296,10 +295,9 @@ void LocalStore::findRoots(const Path & path, unsigned char type, Roots & roots) } else if (type == DT_REG) { - Path storePath = storeDir + "/" + std::string(baseNameOf(path)); - // FIXME - if (isStorePath(storePath) && isValidPath(parseStorePath(storePath))) - roots[parseStorePath(storePath)].emplace(path); + auto storePath = maybeParseStorePath(storeDir + "/" + std::string(baseNameOf(path))); + if (storePath && isValidPath(*storePath)) + roots[std::move(*storePath)].emplace(path); } } @@ -523,14 +521,14 @@ void LocalStore::deletePathRecursive(GCState & state, const Path & path) unsigned long long size = 0; - // FIXME - if (isStorePath(path) && isValidPath(parseStorePath(path))) { + auto storePath = maybeParseStorePath(path); + if (storePath && isValidPath(*storePath)) { StorePathSet referrers; - queryReferrers(parseStorePath(path), referrers); + queryReferrers(*storePath, referrers); for (auto & i : referrers) if (printStorePath(i) != path) deletePathRecursive(state, printStorePath(i)); - size = queryPathInfo(parseStorePath(path))->narSize; - invalidatePathChecked(parseStorePath(path)); + size = queryPathInfo(*storePath)->narSize; + invalidatePathChecked(*storePath); } Path realPath = realStoreDir + "/" + std::string(baseNameOf(path)); @@ -593,8 +591,7 @@ bool LocalStore::canReachRoot(GCState & state, StorePathSet & visited, const Sto visited.insert(path.clone()); - //FIXME - if (!isStorePath(printStorePath(path)) || !isValidPath(path)) return false; + if (!isValidPath(path)) return false; StorePathSet incoming; @@ -637,8 +634,9 @@ void LocalStore::tryToDelete(GCState & state, const Path & path) //Activity act(*logger, lvlDebug, format("considering whether to delete '%1%'") % path); - // FIXME - if (!isStorePath(path) || !isValidPath(parseStorePath(path))) { + auto storePath = maybeParseStorePath(path); + + if (!storePath || !isValidPath(*storePath)) { /* A lock file belonging to a path that we're building right now isn't garbage. */ if (isActiveTempFile(state, path, ".lock")) return; @@ -655,7 +653,7 @@ void LocalStore::tryToDelete(GCState & state, const Path & path) StorePathSet visited; - if (canReachRoot(state, visited, parseStorePath(path))) { + if (storePath && canReachRoot(state, visited, *storePath)) { debug("cannot delete '%s' because it's still reachable", path); } else { /* No path we visited was a root, so everything is garbage. @@ -818,8 +816,8 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) string name = dirent->d_name; if (name == "." || name == "..") continue; Path path = storeDir + "/" + name; - // FIXME - if (isStorePath(path) && isValidPath(parseStorePath(path))) + auto storePath = maybeParseStorePath(path); + if (storePath && isValidPath(*storePath)) entries.push_back(path); else tryToDelete(state, path); diff --git a/src/libstore/path.cc b/src/libstore/path.cc index a33bec3ed..70b919adc 100644 --- a/src/libstore/path.cc +++ b/src/libstore/path.cc @@ -55,6 +55,20 @@ StorePath Store::parseStorePath(std::string_view path) const return StorePath::make(path, storeDir); } +std::optional Store::maybeParseStorePath(std::string_view path) const +{ + try { + return parseStorePath(path); + } catch (Error &) { + return {}; + } +} + +bool Store::isStorePath(const Path & path) const +{ + return (bool) maybeParseStorePath(path); +} + StorePathSet Store::parseStorePathSet(const PathSet & paths) const { StorePathSet res; diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index d8f6c22bc..75fa5d1e6 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -19,14 +19,6 @@ bool Store::isInStore(const Path & path) const } -bool Store::isStorePath(const Path & path) const -{ - return isInStore(path) - && path.size() >= storeDir.size() + 1 + storePathHashLen - && path.find('/', storeDir.size() + 1) == Path::npos; -} - - Path Store::toStorePath(const Path & path) const { if (!isInStore(path)) diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 861b96930..a1cfb314a 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -281,6 +281,8 @@ public: StorePath parseStorePath(std::string_view path) const; + std::optional maybeParseStorePath(std::string_view path) const; + std::string printStorePath(const StorePath & path) const; // FIXME: remove