Merge pull request #5898 from layus/repair-path-links

Make --repair-path also repair corrupt optimised links

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 8dd1965e1..e4d2add73 100644
--- a/src/libstore/build/local-derivation-goal.cc
+++ b/src/libstore/build/local-derivation-goal.cc
@@ -2467,7 +2467,7 @@ void LocalDerivationGoal::registerOutputs()
         }
 
         if (curRound == nrRounds) {
-            localStore.optimisePath(actualPath); // FIXME: combine with scanForReferences()
+            localStore.optimisePath(actualPath, NoRepair); // 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..c4d7b80bd 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, RepairFlag 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, 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 d95e54af1..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)
+    const Path & path, InodeHash & inodeHash, RepairFlag 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, NoRepair);
         }
         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, RepairFlag 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<bool> 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