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 3bdea92f1..67004387c 100644 --- a/src/libstore/optimise-store.cc +++ b/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 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 cc87375f8..9e12f1230 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" @@ -617,6 +618,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 08ec54632..1c8d36a3a 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -303,6 +303,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. */