Merge "libstore: replace random() calls with atomic counter" into main

doc/manual/rl-next/libstore-random.md

@@ -0,0 +1,13 @@
+---
+synopsis: Fix potential store corruption with auto-optimise-store
+issues: [7273]
+cls: [2100]
+category: Fixes
+credits: lilyball
+---
+
+Optimising store paths (and other operations involving temporary files) no longer use `random(3)`
+to generate filenames. On darwin systems this was observed to potentially cause store corruption
+when using [`auto-optimise-store`](@docroot@/command-ref/conf-file.md#conf-auto-optimise-store),
+though this corruption was possible on any system whose `random(3)` does not have locking around
+the global state.

src/libmain/shared.cc

@@ -172,12 +172,6 @@ void initNix()
        now.  In particular, store objects should be readable by
        everybody. */
     umask(0022);
-
-    /* Initialise the PRNG. */
-    struct timeval tv;
-    gettimeofday(&tv, 0);
-    srandom(tv.tv_usec);
-
 }
 
 

src/libstore/build/derivation-goal.cc

@@ -1,4 +1,5 @@
 #include "derivation-goal.hh"
+#include "file-system.hh"
 #include "hook-instance.hh"
 #include "worker.hh"
 #include "finally.hh"
@@ -860,15 +861,22 @@ void replaceValidPath(const Path & storePath, const Path & tmpPath)
        tmpPath (the replacement), so we have to move it out of the
        way first.  We'd better not be interrupted here, because if
        we're repairing (say) Glibc, we end up with a broken system. */
-    Path oldPath = fmt("%1%.old-%2%-%3%", storePath, getpid(), random());
-    if (pathExists(storePath))
+    Path oldPath;
+    if (pathExists(storePath)) {
+        do {
+            oldPath = makeTempPath(storePath, ".old");
+            // store paths are often directories so we can't just unlink() it
+            // let's make sure the path doesn't exist before we try to use it
+        } while (pathExists(oldPath));
         movePath(storePath, oldPath);
+    }
 
     try {
         movePath(tmpPath, storePath);
     } catch (...) {
         try {
             // attempt to recover
+            if (!oldPath.empty())
                 movePath(oldPath, storePath);
         } catch (...) {
             ignoreExceptionExceptInterrupt();
@@ -876,6 +884,7 @@ void replaceValidPath(const Path & storePath, const Path & tmpPath)
         throw;
     }
 
+    if (!oldPath.empty())
         deletePath(oldPath);
 }
 

src/libstore/gc.cc

@@ -33,7 +33,8 @@ static void makeSymlink(const Path & link, const Path & target)
     createDirs(dirOf(link));
 
     /* Create the new symlink. */
-    Path tempLink = fmt("%1%.tmp-%2%-%3%", link, getpid(), random());
+    Path tempLink = makeTempPath(link);
+    unlink(tempLink.c_str()); // just in case; ignore errors
     createSymlink(target, tempLink);
 
     /* Atomically replace the old one. */

src/libstore/optimise-store.cc

@@ -215,7 +215,8 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats,
        its timestamp back to 0. */
     MakeReadOnly makeReadOnly(mustToggle ? dirOfPath : "");
 
-    Path tempLink = fmt("%1%/.tmp-link-%2%-%3%", realStoreDir, getpid(), random());
+    Path tempLink = makeTempPath(realStoreDir, "/.tmp-link");
+    unlink(tempLink.c_str()); // just in case; ignore errors
 
     if (link(linkPath.c_str(), tempLink.c_str()) == -1) {
         if (errno == EMLINK) {
@@ -234,7 +235,7 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats,
         renameFile(tempLink, path);
     } catch (SysError & e) {
         if (unlink(tempLink.c_str()) == -1)
-            printError("unable to unlink '%1%'", tempLink);
+            printError("unable to unlink '%1%': %2%", tempLink, strerror(errno));
         if (errno == EMLINK) {
             /* Some filesystems generate too many links on the rename,
                rather than on the original link.  (Probably it

src/libstore/sqlite.cc

@@ -5,6 +5,7 @@
 #include "signals.hh"
 #include "url.hh"
 
+#include <random>
 #include <sqlite3.h>
 
 
@@ -265,9 +266,11 @@ void handleSQLiteBusy(const SQLiteBusy & e, time_t & nextWarning)
     /* Sleep for a while since retrying the transaction right away
        is likely to fail again. */
     checkInterrupt();
+    static thread_local std::default_random_engine generator(clock());
+    std::uniform_int_distribution<long> uniform_dist(0, 100);
     struct timespec t;
     t.tv_sec = 0;
-    t.tv_nsec = (random() % 100) * 1000 * 1000; /* <= 0.1s */
+    t.tv_nsec = uniform_dist(generator) * 1000 * 1000; /* <= 0.1s */
     nanosleep(&t, 0);
 }
 

src/libutil/file-system.cc

@@ -2,6 +2,7 @@
 #include <cstdlib>
 #include <filesystem>
 #include <atomic>
+#include <random>
 
 #include "environment-variables.hh"
 #include "file-descriptor.hh"
@@ -617,6 +618,13 @@ std::pair<AutoCloseFD, Path> createTempFile(const Path & prefix)
     return {std::move(fd), tmpl};
 }
 
+Path makeTempPath(const Path & root, const Path & suffix)
+{
+    // start the counter at a random value to minimize issues with preexisting temp paths
+    static std::atomic_uint_fast32_t counter(std::random_device{}());
+    return fmt("%1%%2%-%3%-%4%", root, suffix, getpid(), counter.fetch_add(1, std::memory_order_relaxed));
+}
+
 void createSymlink(const Path & target, const Path & link)
 {
     if (symlink(target.c_str(), link.c_str()))

src/libutil/file-system.hh

@@ -303,6 +303,14 @@ std::pair<AutoCloseFD, Path> createTempFile(const Path & prefix = "nix");
  */
 Path defaultTempDir();
 
+/**
+ * Return temporary path constructed by appending a suffix to a root path.
+ *
+ * The constructed path looks like `<root><suffix>-<pid>-<unique>`. To create a
+ * path nested in a directory, provide a suffix starting with `/`.
+ */
+Path makeTempPath(const Path & root, const Path & suffix = ".tmp");
+
 /**
  * Used in various places.
  */