From ef05a2fa0f7bb70e5203c5ee4c6cb143ae86372d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Thu, 12 Jun 2025 17:40:11 +0200 Subject: [PATCH 1/4] optimise-store: Fix race condition in temporary link creation Replace rand() with atomic counter for unique temp link names and add retry loop to handle EEXIST errors when multiple processes optimize the store simultaneously. --- src/libstore/optimise-store.cc | 71 ++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 28 deletions(-) diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index e47c0707c..28a64cae8 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -223,38 +223,53 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, its timestamp back to 0. */ MakeReadOnly makeReadOnly(mustToggle ? dirOfPath : ""); - std::filesystem::path tempLink = fmt("%1%/.tmp-link-%2%-%3%", config->realStoreDir, getpid(), rand()); + /* Retry loop for temporary link creation to handle race conditions */ + while (true) { + Path tempLink = makeTempPath(config->realStoreDir.get(), ".tmp-link"); - try { - std::filesystem::create_hard_link(linkPath, tempLink); - inodeHash.insert(st.st_ino); - } catch (std::filesystem::filesystem_error & e) { - if (e.code() == std::errc::too_many_links) { - /* Too many links to the same file (>= 32000 on most file - systems). This is likely to happen with empty files. - Just shrug and ignore. */ - if (st.st_size) - printInfo("'%1%' has maximum number of links", linkPath); - return; + try { + std::filesystem::create_hard_link(linkPath, tempLink); + inodeHash.insert(st.st_ino); + } catch (std::filesystem::filesystem_error & e) { + if (e.code() == std::errc::too_many_links) { + /* Too many links to the same file (>= 32000 on most file + systems). This is likely to happen with empty files. + Just shrug and ignore. */ + if (st.st_size) + printInfo("'%1%' has maximum number of links", linkPath); + return; + } + throw SysError("creating temporary link '%1%'", tempLink); } - throw; - } - /* Atomically replace the old file with the new hard link. */ - try { - std::filesystem::rename(tempLink, path); - } catch (std::filesystem::filesystem_error & e) { - std::filesystem::remove(tempLink); - printError("unable to unlink '%1%'", tempLink); - if (e.code() == std::errc::too_many_links) { - /* Some filesystems generate too many links on the rename, - rather than on the original link. (Probably it - temporarily increases the st_nlink field before - decreasing it again.) */ - debug("'%s' has reached maximum number of links", linkPath); - return; + /* Atomically replace the old file with the new hard link. */ + try { + std::filesystem::rename(tempLink, path); + break; /* Success! */ + } catch (std::filesystem::filesystem_error & e) { + try { + std::filesystem::remove(tempLink); + } catch (...) { + /* Ignore errors removing the temp link */ + } + + if (e.code() == std::errc::too_many_links) { + /* Some filesystems generate too many links on the rename, + rather than on the original link. (Probably it + temporarily increases the st_nlink field before + decreasing it again.) */ + debug("%s has reached maximum number of links", linkPath); + return; + } + + if (e.code() == std::errc::file_exists) { + /* Race condition: another process created the same temp link. + Try again with a different name. */ + continue; + } + + throw SysError("renaming temporary link '%1%' to '%2%'", tempLink, path); } - throw; } stats.filesLinked++; From 975b2c2edd0c14181f6d9a52b3bc64fe2a5cf599 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Thu, 12 Jun 2025 17:40:30 +0200 Subject: [PATCH 2/4] indirect-root-store: Fix race condition in makeSymlink Replace rand() with atomic counter and add retry loop to handle race conditions when creating temporary symlinks for garbage collector roots. --- src/libstore/indirect-root-store.cc | 31 ++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/src/libstore/indirect-root-store.cc b/src/libstore/indirect-root-store.cc index e23c01e5d..6e82332db 100644 --- a/src/libstore/indirect-root-store.cc +++ b/src/libstore/indirect-root-store.cc @@ -1,4 +1,5 @@ #include "nix/store/indirect-root-store.hh" +#include "nix/util/file-system.hh" namespace nix { @@ -7,12 +8,32 @@ void IndirectRootStore::makeSymlink(const Path & link, const Path & target) /* Create directories up to `gcRoot'. */ createDirs(dirOf(link)); - /* Create the new symlink. */ - Path tempLink = fmt("%1%.tmp-%2%-%3%", link, getpid(), rand()); - createSymlink(target, tempLink); + /* Retry loop for temporary symlink creation to handle race conditions */ + while (true) { + Path tempLink = makeTempPath(dirOf(link), baseNameOf(link) + ".tmp"); - /* Atomically replace the old one. */ - std::filesystem::rename(tempLink, link); + createSymlink(target, tempLink); + + /* Atomically replace the old one. */ + try { + std::filesystem::rename(tempLink, link); + break; /* Success! */ + } catch (std::filesystem::filesystem_error & e) { + try { + std::filesystem::remove(tempLink); + } catch (...) { + /* Ignore errors removing the temp link */ + } + + if (e.code() == std::errc::file_exists) { + /* Race condition: another process created the same temp link. + Try again with a different name. */ + continue; + } + + throw SysError("failed to create symlink '%1%' -> '%2%'", link, target); + } + } } Path IndirectRootStore::addPermRoot(const StorePath & storePath, const Path & _gcRoot) From bc73faf88f3629eb05ea111bfe303f4cae678797 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Thu, 12 Jun 2025 17:40:54 +0200 Subject: [PATCH 3/4] local-store: Fix potential race in build log creation Add atomic counter to temporary log file names and retry loop to handle EEXIST errors. If another process creates the log file first, we gracefully exit since the log already exists. --- src/libstore/local-store.cc | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 76fadba86..fc7798644 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -1666,11 +1667,35 @@ void LocalStore::addBuildLog(const StorePath & drvPath, std::string_view log) createDirs(dirOf(logPath)); - auto tmpFile = fmt("%s.tmp.%d", logPath, getpid()); + /* Retry loop for temporary log file creation to handle race conditions */ + while (true) { + auto tmpFile = makeTempPath(dirOf(logPath), baseNameOf(logPath) + ".tmp"); - writeFile(tmpFile, compress("bzip2", log)); + try { + writeFile(tmpFile, compress("bzip2", log)); + } catch (Error & e) { + e.addTrace({}, "writing build log to '%s'", tmpFile); + throw; + } - std::filesystem::rename(tmpFile, logPath); + try { + std::filesystem::rename(tmpFile, logPath); + break; /* Success! */ + } catch (std::filesystem::filesystem_error & e) { + try { + std::filesystem::remove(tmpFile); + } catch (...) { + /* Ignore errors removing the temp file */ + } + + if (e.code() == std::errc::file_exists) { + /* Another process created the log file. That's fine, we're done. */ + break; + } + + throw SysError("renaming temporary file '%1%' to '%2%'", tmpFile, logPath); + } + } } std::optional LocalStore::getVersion() From f124293024877083861aaa4ef82e0b93038eb529 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Thu, 12 Jun 2025 17:41:13 +0200 Subject: [PATCH 4/4] local-binary-cache-store: Add retry loop to upsertFile Handle race conditions in binary cache file creation by retrying with a new temporary name on EEXIST errors. Uses existing makeTempPath which already has an atomic counter. --- src/libstore/local-binary-cache-store.cc | 33 +++++++++++++++++++----- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index 2f23135fa..97866c0ca 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -58,13 +58,32 @@ protected: const std::string & mimeType) override { auto path2 = config->binaryCacheDir + "/" + path; - static std::atomic counter{0}; - Path tmp = fmt("%s.tmp.%d.%d", path2, getpid(), ++counter); - AutoDelete del(tmp, false); - StreamToSourceAdapter source(istream); - writeFile(tmp, source); - std::filesystem::rename(tmp, path2); - del.cancel(); + + /* Retry loop for handling race conditions */ + while (true) { + Path tmp = makeTempPath(dirOf(path2), baseNameOf(path2) + ".tmp"); + AutoDelete del(tmp, false); + + StreamToSourceAdapter source(istream); + try { + writeFile(tmp, source); + } catch (Error & e) { + e.addTrace({}, "while writing to temporary file '%s' for '%s'", tmp, path2); + } + + try { + std::filesystem::rename(tmp, path2); + del.cancel(); + break; /* Success! */ + } catch (std::filesystem::filesystem_error & e) { + if (e.code() == std::errc::file_exists) { + /* Race condition: another process created the same file. + Try again with a different name. */ + continue; + } + throw SysError("renaming '%s' to '%s'", tmp, path2); + } + } } void getFile(const std::string & path, Sink & sink) override