From 69957a971e0e1494741623e926f61cf75cbce3e1 Mon Sep 17 00:00:00 2001 From: Lily Ballard Date: Sun, 20 Oct 2024 20:35:01 -0700 Subject: [PATCH] libstore: replace random() calls with atomic counter random() is not thread-safe, it relies on global state, and calling it from worker threads can result in multiple threads producing the same value. It also doesn't guarantee unique values even in single-threaded use. Use an atomic counter for the use-case of generating temporary paths, and switch to a thread-local RNG for the one remaining call. This will probably fix https://github.com/NixOS/nix/issues/7273 though I'm not willing to risk corrupting my store to find out. Change-Id: I4c4c4c9796613573ffefd29cc8efe3d07839facc --- doc/manual/rl-next/libstore-random.md | 13 +++++++++++++ src/libmain/shared.cc | 6 ------ src/libstore/build/derivation-goal.cc | 17 +++++++++++++---- src/libstore/gc.cc | 3 ++- src/libstore/optimise-store.cc | 5 +++-- src/libstore/sqlite.cc | 5 ++++- src/libutil/file-system.cc | 8 ++++++++ src/libutil/file-system.hh | 8 ++++++++ 8 files changed, 51 insertions(+), 14 deletions(-) create mode 100644 doc/manual/rl-next/libstore-random.md diff --git a/doc/manual/rl-next/libstore-random.md b/doc/manual/rl-next/libstore-random.md new file mode 100644 index 000000000..e67e37c07 --- /dev/null +++ b/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. diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 77c497237..2615c34b1 100644 --- a/src/libmain/shared.cc +++ b/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); - } diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 765df5f5a..d54c08b3b 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/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,23 +861,31 @@ 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 - movePath(oldPath, storePath); + if (!oldPath.empty()) + movePath(oldPath, storePath); } catch (...) { ignoreExceptionExceptInterrupt(); } throw; } - deletePath(oldPath); + if (!oldPath.empty()) + deletePath(oldPath); } diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 99bf80994..72a32823a 100644 --- a/src/libstore/gc.cc +++ b/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. */ diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index 31cb3d303..1b4f74a1e 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -214,7 +214,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) { @@ -233,7 +234,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 diff --git a/src/libstore/sqlite.cc b/src/libstore/sqlite.cc index 7aa0b6629..2e010c30e 100644 --- a/src/libstore/sqlite.cc +++ b/src/libstore/sqlite.cc @@ -5,6 +5,7 @@ #include "signals.hh" #include "url.hh" +#include #include @@ -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 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); } diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index c4ffb1d0c..67434bd4a 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -2,6 +2,7 @@ #include #include #include +#include #include "environment-variables.hh" #include "file-descriptor.hh" @@ -595,6 +596,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) { if (symlink(target.c_str(), link.c_str())) diff --git a/src/libutil/file-system.hh b/src/libutil/file-system.hh index 2547c63b5..3a65a0f56 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -296,6 +296,14 @@ std::pair createTempFile(const Path & prefix = "nix"); */ Path defaultTempDir(); +/** + * 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. */