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) 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 diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index e53cab2dc..05eb7ec63 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() 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++;