cherry-pick https://gerrit.lix.systems/c/lix/+/2100

Cherry-pick https://gerrit.lix.systems/c/lix/+/2100

This change fixes a potential concurrency failure when accessing random
which is not thread safe.

Co-authored-by: Lily Ballard <lily@ballards.net>

src/libmain/shared.cc

@@ -176,16 +176,6 @@ void initNix(bool loadConfig)
        now.  In particular, store objects should be readable by
        everybody. */
     umask(0022);
-
-    /* Initialise the PRNG. */
-    struct timeval tv;
-    gettimeofday(&tv, 0);
-#ifndef _WIN32
-    srandom(tv.tv_usec);
-#endif
-    srand(tv.tv_usec);
-
-
 }
 
 

src/libstore/build/derivation-goal.cc

@@ -9,7 +9,7 @@
 #include "nix/util/util.hh"
 #include "nix/util/compression.hh"
 #include "nix/store/common-protocol.hh"
-#include "nix/store/common-protocol-impl.hh"
+#include "nix/store/common-protocol-impl.hh" // Don't remove is actually needed
 #include "nix/store/local-store.hh" // TODO remove, along with remaining downcasts
 
 #include <fstream>

src/libstore/unix/build/derivation-builder.cc

@@ -1,22 +1,14 @@
 #include "nix/store/build/derivation-builder.hh"
 #include "nix/store/local-store.hh"
 #include "nix/util/processes.hh"
-#include "nix/store/indirect-root-store.hh"
-#include "nix/store/build/hook-instance.hh"
-#include "nix/store/build/worker.hh"
 #include "nix/store/builtins.hh"
-#include "nix/store/builtins/buildenv.hh"
 #include "nix/store/path-references.hh"
 #include "nix/util/finally.hh"
 #include "nix/util/util.hh"
 #include "nix/util/archive.hh"
 #include "nix/util/git.hh"
-#include "nix/util/compression.hh"
 #include "nix/store/daemon.hh"
 #include "nix/util/topo-sort.hh"
-#include "nix/util/callback.hh"
-#include "nix/util/json-utils.hh"
-#include "nix/util/current-process.hh"
 #include "nix/store/build/child.hh"
 #include "nix/util/unix-domain-socket.hh"
 #include "nix/store/posix-fs-canonicalise.hh"
@@ -671,23 +663,33 @@ static 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(), rand());
+    Path oldPath;
-    if (pathExists(storePath))
-        movePath(storePath, oldPath);
     
+    if (pathExists(storePath)) {
+        // why do we loop here?
+        // although makeTempPath should be unique, we can't 
+        // guarantee that.
+        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
-            movePath(oldPath, storePath);
+            if (!oldPath.empty())
+                movePath(oldPath, storePath);
         } catch (...) {
             ignoreExceptionExceptInterrupt();
         }
         throw;
     }
-
+    if (!oldPath.empty())
-    deletePath(oldPath);
+        deletePath(oldPath);
 }
 
 

src/libstore/unix/user-lock.cc

@@ -7,6 +7,7 @@
 #include "nix/store/globals.hh"
 #include "nix/store/pathlocks.hh"
 #include "nix/util/users.hh"
+#include "nix/util/logging.hh"
 
 namespace nix {
 

src/libutil/file-system.cc

@@ -8,12 +8,12 @@
 #include "nix/util/util.hh"
 
 #include <atomic>
+#include <random>
 #include <cerrno>
 #include <climits>
 #include <cstdio>
 #include <cstdlib>
 #include <deque>
-#include <sstream>
 #include <filesystem>
 
 #include <fcntl.h>
@@ -27,10 +27,6 @@
 # include <io.h>
 #endif
 
-#include "nix/util/strings-inline.hh"
-
-#include "util-config-private.hh"
-
 namespace nix {
 
 DirectoryIterator::DirectoryIterator(const std::filesystem::path& p) {
@@ -641,6 +637,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)
 {
     try {

src/libutil/include/nix/util/file-system.hh

@@ -6,8 +6,6 @@
  */
 
 #include "nix/util/types.hh"
-#include "nix/util/error.hh"
-#include "nix/util/logging.hh"
 #include "nix/util/file-descriptor.hh"
 #include "nix/util/file-path.hh"
 
@@ -18,12 +16,8 @@
 #ifdef _WIN32
 # include <windef.h>
 #endif
-#include <signal.h>
 
-#include <atomic>
 #include <functional>
-#include <map>
-#include <sstream>
 #include <optional>
 
 /**
@@ -335,6 +329,14 @@ Path defaultTempDir();
  */
 bool isExecutableFileAmbient(const std::filesystem::path & exe);
 
+/**
+ * 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.
  */

src/libutil/source-accessor.cc

@@ -1,5 +1,5 @@
+#include <atomic>
 #include "nix/util/source-accessor.hh"
-#include "nix/util/archive.hh"
 
 namespace nix {