From 2a5f5fbb1776f5086646407e5296c372321863b9 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 31 Jul 2023 11:13:38 -0400 Subject: [PATCH 1/4] `LocalStore::verifyPath`: Use `StorePathSet` for `store` local var We don't care about non-store-paths in there (things like `.links`, are, in fact, allowed). So let's just skip them up front and be more strongly typed. --- src/libstore/local-store.cc | 12 ++++++++---- src/libstore/local-store.hh | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index f78bd44ca..9049f33aa 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1499,8 +1499,12 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) auto fdGCLock = openGCLock(); FdLock gcLock(fdGCLock.get(), ltRead, true, "waiting for the big garbage collector lock..."); - StringSet store; - for (auto & i : readDirectory(realStoreDir)) store.insert(i.name); + StorePathSet store; + for (auto & i : readDirectory(realStoreDir)) { + try { + store.insert({i.name}); + } catch (BadStorePath &) { } + } /* Check whether all valid paths actually exist. */ printInfo("checking path existence..."); @@ -1595,14 +1599,14 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) } -void LocalStore::verifyPath(const StorePath & path, const StringSet & store, +void LocalStore::verifyPath(const StorePath & path, const StorePathSet & store, StorePathSet & done, StorePathSet & validPaths, RepairFlag repair, bool & errors) { checkInterrupt(); if (!done.insert(path).second) return; - if (!store.count(std::string(path.to_string()))) { + if (!store.count(path)) { /* Check any referrers first. If we can invalidate them first, then we can invalidate this path as well. */ bool canInvalidate = true; diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index c9b570eaa..e97195f5b 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -314,7 +314,7 @@ private: */ void invalidatePathChecked(const StorePath & path); - void verifyPath(const StorePath & path, const StringSet & store, + void verifyPath(const StorePath & path, const StorePathSet & store, StorePathSet & done, StorePathSet & validPaths, RepairFlag repair, bool & errors); std::shared_ptr queryPathInfoInternal(State & state, const StorePath & path); From 6525265f4640221efb0039ddbd6849a3b04babc9 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 31 Jul 2023 12:22:06 -0400 Subject: [PATCH 2/4] `LocalStore::verifyPath`: Try to clarify data flow with more scopes It was initially unclear to me which of these are temporary state for the verify paths computation, and which of these are the results of that computation to be used in the rest of the function. Now, it is clear, and enforced. --- src/libstore/local-store.cc | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 9049f33aa..17e2ebc38 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1499,21 +1499,24 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) auto fdGCLock = openGCLock(); FdLock gcLock(fdGCLock.get(), ltRead, true, "waiting for the big garbage collector lock..."); - StorePathSet store; - for (auto & i : readDirectory(realStoreDir)) { - try { - store.insert({i.name}); - } catch (BadStorePath &) { } - } - - /* Check whether all valid paths actually exist. */ - printInfo("checking path existence..."); - StorePathSet validPaths; - StorePathSet done; - for (auto & i : queryAllValidPaths()) - verifyPath(i, store, done, validPaths, repair, errors); + { + StorePathSet store; + for (auto & i : readDirectory(realStoreDir)) { + try { + store.insert({i.name}); + } catch (BadStorePath &) { } + } + + /* Check whether all valid paths actually exist. */ + printInfo("checking path existence..."); + + StorePathSet done; + + for (auto & i : queryAllValidPaths()) + verifyPath(i, store, done, validPaths, repair, errors); + } /* Optionally, check the content hashes (slow). */ if (checkContents) { From 770d50e49cce4d8ce5e546fe31beaa253505bfa5 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 2 Aug 2023 12:40:04 -0400 Subject: [PATCH 3/4] local-store verifying: Rename `store` to something more clear It is not a `Store` but a `StorePathSet`. --- src/libstore/local-store.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 17e2ebc38..982a9059c 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1502,10 +1502,10 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) StorePathSet validPaths; { - StorePathSet store; + StorePathSet storePathsInStoreDir; for (auto & i : readDirectory(realStoreDir)) { try { - store.insert({i.name}); + storePathsInStoreDir.insert({i.name}); } catch (BadStorePath &) { } } @@ -1515,7 +1515,7 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) StorePathSet done; for (auto & i : queryAllValidPaths()) - verifyPath(i, store, done, validPaths, repair, errors); + verifyPath(i, storePathsInStoreDir, done, validPaths, repair, errors); } /* Optionally, check the content hashes (slow). */ @@ -1602,21 +1602,21 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) } -void LocalStore::verifyPath(const StorePath & path, const StorePathSet & store, +void LocalStore::verifyPath(const StorePath & path, const StorePathSet & storePathsInStoreDir, StorePathSet & done, StorePathSet & validPaths, RepairFlag repair, bool & errors) { checkInterrupt(); if (!done.insert(path).second) return; - if (!store.count(path)) { + if (!storePathsInStoreDir.count(path)) { /* Check any referrers first. If we can invalidate them first, then we can invalidate this path as well. */ bool canInvalidate = true; StorePathSet referrers; queryReferrers(path, referrers); for (auto & i : referrers) if (i != path) { - verifyPath(i, store, done, validPaths, repair, errors); + verifyPath(i, storePathsInStoreDir, done, validPaths, repair, errors); if (validPaths.count(i)) canInvalidate = false; } From 66550878dffe2a4bf55ea7ab694738aa14e6a01e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 2 Aug 2023 12:45:55 -0400 Subject: [PATCH 4/4] Add comment explaining the use of `readDirectory(realStoreDir)` --- src/libstore/local-store.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 982a9059c..40a3bc194 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1503,6 +1503,15 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) { StorePathSet storePathsInStoreDir; + /* Why aren't we using `queryAllValidPaths`? Because that would + tell us about all the paths than the database knows about. Here we + want to know about all the store paths in the store directory, + regardless of what the database thinks. + + We will end up cross-referencing these two sources of truth (the + database and the filesystem) in the loop below, in order to catch + invalid states. + */ for (auto & i : readDirectory(realStoreDir)) { try { storePathsInStoreDir.insert({i.name});