From 6aed9d877cfbe1e649b69d2f15b6aeccbfbb0f3a Mon Sep 17 00:00:00 2001 From: Farid Zakaria Date: Tue, 20 May 2025 19:13:49 -0700 Subject: [PATCH] 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 --- src/libmain/shared.cc | 10 ------- src/libstore/build/derivation-goal.cc | 2 +- src/libstore/unix/build/derivation-builder.cc | 30 ++++++++++--------- src/libstore/unix/user-lock.cc | 1 + src/libutil/file-system.cc | 13 ++++---- src/libutil/include/nix/util/file-system.hh | 14 +++++---- src/libutil/source-accessor.cc | 2 +- 7 files changed, 35 insertions(+), 37 deletions(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 50d4991be..2da22a773 100644 --- a/src/libmain/shared.cc +++ b/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); - - } diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 82c350832..9763a5cd7 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/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 diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index f3847e604..f107f7a5d 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/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()); - if (pathExists(storePath)) + Path 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; } - - deletePath(oldPath); + if (!oldPath.empty()) + deletePath(oldPath); } diff --git a/src/libstore/unix/user-lock.cc b/src/libstore/unix/user-lock.cc index 2bee277f9..ef806af79 100644 --- a/src/libstore/unix/user-lock.cc +++ b/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 { diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 90ec5eda5..06c9a8c1f 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -8,12 +8,12 @@ #include "nix/util/util.hh" #include +#include #include #include #include #include #include -#include #include #include @@ -27,10 +27,6 @@ # include #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 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 { diff --git a/src/libutil/include/nix/util/file-system.hh b/src/libutil/include/nix/util/file-system.hh index b8fa4cfa0..02af5b37b 100644 --- a/src/libutil/include/nix/util/file-system.hh +++ b/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 #endif -#include -#include #include -#include -#include #include /** @@ -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 `--`. 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. */ diff --git a/src/libutil/source-accessor.cc b/src/libutil/source-accessor.cc index b9ebc82b6..fc9752456 100644 --- a/src/libutil/source-accessor.cc +++ b/src/libutil/source-accessor.cc @@ -1,5 +1,5 @@ +#include #include "nix/util/source-accessor.hh" -#include "nix/util/archive.hh" namespace nix {