From e9a4abdb5d6fe8e128372a77d879b0187b1bacfe Mon Sep 17 00:00:00 2001 From: Guillaume Maudoux Date: Tue, 11 Jan 2022 11:57:45 +0100 Subject: [PATCH 1/2] Make --repair-path also repair corrupt optimised links There already existed a smoke test for the link content length, but it appears that there exists some corruptions pernicious enough to replace the file content with zeros, and keeping the same length. --repair-path now goes as far as checking the content of the link, making it true to its name and actually repairing the path for such coruption cases. --- src/libstore/build/local-derivation-goal.cc | 2 +- src/libstore/local-store.cc | 6 +-- src/libstore/local-store.hh | 7 +-- src/libstore/optimise-store.cc | 31 ++++++++------ src/libutil/logging.hh | 2 +- tests/repair.sh | 47 +++++++++++++++++++++ 6 files changed, 75 insertions(+), 20 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 66d950b42..9ad28c6a9 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2460,7 +2460,7 @@ void LocalDerivationGoal::registerOutputs() } if (curRound == nrRounds) { - localStore.optimisePath(actualPath); // FIXME: combine with scanForReferences() + localStore.optimisePath(actualPath, false); // FIXME: combine with scanForReferences() worker.markContentsGood(newInfo.path); } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 1b8e77ead..9ebdfd6ed 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1307,7 +1307,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, canonicalisePathMetaData(realPath, -1); - optimisePath(realPath); // FIXME: combine with hashPath() + optimisePath(realPath, repair); // FIXME: combine with hashPath() registerValidPath(info); } @@ -1419,7 +1419,7 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, const string & name, canonicalisePathMetaData(realPath, -1); // FIXME: merge into restorePath - optimisePath(realPath); + optimisePath(realPath, repair); ValidPathInfo info { dstPath, narHash.first }; info.narSize = narHash.second; @@ -1463,7 +1463,7 @@ StorePath LocalStore::addTextToStore(const string & name, const string & s, dumpString(s, sink); auto narHash = hashString(htSHA256, *sink.s); - optimisePath(realPath); + optimisePath(realPath, repair); ValidPathInfo info { dstPath, narHash }; info.narSize = sink.s->size(); diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 115ea046a..04698ba07 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -172,8 +172,9 @@ public: void optimiseStore() override; - /* Optimise a single store path. */ - void optimisePath(const Path & path); + /* Optimise a single store path. Optionally, test the encountered + symlinks for corruption. */ + void optimisePath(const Path & path, bool repair); bool verifyStore(bool checkContents, RepairFlag repair) override; @@ -253,7 +254,7 @@ private: InodeHash loadInodeHash(); Strings readDirectoryIgnoringInodes(const Path & path, const InodeHash & inodeHash); - void optimisePath_(Activity * act, OptimiseStats & stats, const Path & path, InodeHash & inodeHash); + void optimisePath_(Activity * act, OptimiseStats & stats, const Path & path, InodeHash & inodeHash, bool repair); // Internal versions that are not wrapped in retry_sqlite. bool isValidPath_(State & state, const StorePath & path); diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index d95e54af1..c93d0706d 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -88,7 +88,7 @@ Strings LocalStore::readDirectoryIgnoringInodes(const Path & path, const InodeHa void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, - const Path & path, InodeHash & inodeHash) + const Path & path, InodeHash & inodeHash, bool repair) { checkInterrupt(); @@ -110,7 +110,7 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, if (S_ISDIR(st.st_mode)) { Strings names = readDirectoryIgnoringInodes(path, inodeHash); for (auto & i : names) - optimisePath_(act, stats, path + "/" + i, inodeHash); + optimisePath_(act, stats, path + "/" + i, inodeHash, repair); return; } @@ -151,7 +151,20 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, /* Check if this is a known hash. */ Path linkPath = linksDir + "/" + hash.to_string(Base32, false); - retry: + /* Maybe delete the link, if it has been corrupted. */ + if (pathExists(linkPath)) { + auto stLink = lstat(linkPath); + if (st.st_size != stLink.st_size + || (repair && hash != hashPath(htSHA256, linkPath).first)) + { + // XXX: Consider overwriting linkPath with our valid version. + warn("removing corrupted link '%s'", linkPath); + warn("There may be more corrupted paths." + "\nYou should run `nix-store --verify --check-contents --repair` to fix them all"); + unlink(linkPath.c_str()); + } + } + if (!pathExists(linkPath)) { /* Nope, create a hard link in the links directory. */ if (link(path.c_str(), linkPath.c_str()) == 0) { @@ -187,12 +200,6 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, return; } - if (st.st_size != stLink.st_size) { - warn("removing corrupted link '%s'", linkPath); - unlink(linkPath.c_str()); - goto retry; - } - printMsg(lvlTalkative, format("linking '%1%' to '%2%'") % path % linkPath); /* Make the containing directory writable, but only if it's not @@ -260,7 +267,7 @@ void LocalStore::optimiseStore(OptimiseStats & stats) if (!isValidPath(i)) continue; /* path was GC'ed, probably */ { Activity act(*logger, lvlTalkative, actUnknown, fmt("optimising path '%s'", printStorePath(i))); - optimisePath_(&act, stats, realStoreDir + "/" + std::string(i.to_string()), inodeHash); + optimisePath_(&act, stats, realStoreDir + "/" + std::string(i.to_string()), inodeHash, false); } done++; act.progress(done, paths.size()); @@ -278,12 +285,12 @@ void LocalStore::optimiseStore() stats.filesLinked); } -void LocalStore::optimisePath(const Path & path) +void LocalStore::optimisePath(const Path & path, bool repair) { OptimiseStats stats; InodeHash inodeHash; - if (settings.autoOptimiseStore) optimisePath_(nullptr, stats, path, inodeHash); + if (settings.autoOptimiseStore) optimisePath_(nullptr, stats, path, inodeHash, repair); } diff --git a/src/libutil/logging.hh b/src/libutil/logging.hh index ce9c3dfed..5560d2bed 100644 --- a/src/libutil/logging.hh +++ b/src/libutil/logging.hh @@ -40,7 +40,7 @@ struct LoggerSettings : Config Setting showTrace{ this, false, "show-trace", R"( - Where Nix should print out a stack trace in case of Nix + Whether Nix should print out a stack trace in case of Nix expression evaluation errors. )"}; }; diff --git a/tests/repair.sh b/tests/repair.sh index 1105b446b..c8f07b1c6 100644 --- a/tests/repair.sh +++ b/tests/repair.sh @@ -74,3 +74,50 @@ if [ "$(nix-hash $path2)" != "$hash" -o -e $path2/bad ]; then echo "path not repaired properly" >&2 exit 1 fi + +# Check that --repair-path also checks content of optimised symlinks (1/2) +nix-store --verify-path $path2 + +if (! nix-store --optimize); then + echo "nix-store --optimize failed to optimize the store" >&2 + exit 1 +fi +chmod u+w $path2/bar +echo 'rabrab' > $path2/bar # different length + +if nix-store --verify-path $path2; then + echo "nix-store --verify-path did not detect .links file corruption" >&2 + exit 1 +fi + +nix-store --repair-path $path2 --option auto-optimise-store true + +if [ "$(nix-hash $path2)" != "$hash" -o "BAR" != "$(< $path2/bar)" ]; then + echo "path not repaired properly" >&2 + exit 1 +fi + +# Check that --repair-path also checks content of optimised symlinks (2/2) +nix-store --verify-path $path2 + +if (! nix-store --optimize); then + echo "nix-store --optimize failed to optimize the store" >&2 + exit 1 +fi +chmod u+w $path2 +chmod u+w $path2/bar +sed -e 's/./X/g' < $path2/bar > $path2/tmp # same length, different content. +cp $path2/tmp $path2/bar +rm $path2/tmp + +if nix-store --verify-path $path2; then + echo "nix-store --verify-path did not detect .links file corruption" >&2 + exit 1 +fi + +nix-store --repair-path $path2 --substituters "file://$cacheDir" --no-require-sigs --option auto-optimise-store true + +if [ "$(nix-hash $path2)" != "$hash" -o "BAR" != "$(< $path2/bar)" ]; then + echo "path not repaired properly" >&2 + exit 1 +fi From 9f9f39a24b515d5d3ddd2bbd4cdadb0b1924bc3b Mon Sep 17 00:00:00 2001 From: Guillaume Maudoux Date: Tue, 11 Jan 2022 13:34:57 +0100 Subject: [PATCH 2/2] Prefer RepairFlag over bool when applicable --- src/libstore/build/local-derivation-goal.cc | 2 +- src/libstore/local-store.hh | 4 ++-- src/libstore/optimise-store.cc | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 9ad28c6a9..61991a7e5 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2460,7 +2460,7 @@ void LocalDerivationGoal::registerOutputs() } if (curRound == nrRounds) { - localStore.optimisePath(actualPath, false); // FIXME: combine with scanForReferences() + localStore.optimisePath(actualPath, NoRepair); // FIXME: combine with scanForReferences() worker.markContentsGood(newInfo.path); } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 04698ba07..c4d7b80bd 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -174,7 +174,7 @@ public: /* Optimise a single store path. Optionally, test the encountered symlinks for corruption. */ - void optimisePath(const Path & path, bool repair); + void optimisePath(const Path & path, RepairFlag repair); bool verifyStore(bool checkContents, RepairFlag repair) override; @@ -254,7 +254,7 @@ private: InodeHash loadInodeHash(); Strings readDirectoryIgnoringInodes(const Path & path, const InodeHash & inodeHash); - void optimisePath_(Activity * act, OptimiseStats & stats, const Path & path, InodeHash & inodeHash, bool repair); + void optimisePath_(Activity * act, OptimiseStats & stats, const Path & path, InodeHash & inodeHash, RepairFlag repair); // Internal versions that are not wrapped in retry_sqlite. bool isValidPath_(State & state, const StorePath & path); diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index c93d0706d..1833c954e 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -88,7 +88,7 @@ Strings LocalStore::readDirectoryIgnoringInodes(const Path & path, const InodeHa void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, - const Path & path, InodeHash & inodeHash, bool repair) + const Path & path, InodeHash & inodeHash, RepairFlag repair) { checkInterrupt(); @@ -267,7 +267,7 @@ void LocalStore::optimiseStore(OptimiseStats & stats) if (!isValidPath(i)) continue; /* path was GC'ed, probably */ { Activity act(*logger, lvlTalkative, actUnknown, fmt("optimising path '%s'", printStorePath(i))); - optimisePath_(&act, stats, realStoreDir + "/" + std::string(i.to_string()), inodeHash, false); + optimisePath_(&act, stats, realStoreDir + "/" + std::string(i.to_string()), inodeHash, NoRepair); } done++; act.progress(done, paths.size()); @@ -285,7 +285,7 @@ void LocalStore::optimiseStore() stats.filesLinked); } -void LocalStore::optimisePath(const Path & path, bool repair) +void LocalStore::optimisePath(const Path & path, RepairFlag repair) { OptimiseStats stats; InodeHash inodeHash;