From 2a17164865d43e5022009f7f035cd866ee775e50 Mon Sep 17 00:00:00 2001 From: Emily Date: Sun, 3 Aug 2025 19:05:32 +0100 Subject: [PATCH] libutil: use `makeTempPath` in `createTempSubdir` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes the paths more nondeterministic, but more reliably unique, and lets us remove the retry loop. Note that this adds random entropy to the build directory visible inside derivations on Darwin and unsandboxed Linux. It was already non‐deterministic in the presence of concurrent builds and similar, but now we can reliably expect it to be different every time. On the whole I think that’s a good thing, as it is impossible to ensure a single consistent build directory and derivation outputs should not depend on it. Package reproducibility isn’t great on Darwin to begin with, though, and the reproducibility bugs this will turn up in packages will be more urgent to fix than when the build directory was mostly consistent. A quick survey of my local store shows that many C, C++, and Rust binaries contain build directory references, likely due to use of `__FILE__` and its equivalents; non‐binary offenders include: * Install logs included in the Rust and Cargo bootstrap compilers * Example errors in the Rust documentation referencing build paths * Configuration information installed with CPython itself * Python 2 metadata from resholve’s closure * Cython metadata * Generated headers in Facebook libraries referencing source paths * Generated CMake files in Facebook libraries referencing source paths I haven’t built that much in this store since the last GC, so this is probably only a small sample of the problems across the tree. These are all instances of , though, and should probably just be treated as general reproducibility bugs outside of contexts like the Linux sandbox where we can normalize them away entirely. I have implemented away build directory paths for C/C++, applied some additional fixes for non‐`__FILE__`‐related issues in binaries from ATF and LLVM, and fixed the derivation bug causing the CPython 3 issue, and will work on upstreaming these changes. Rust is working on the problem upstream, with some temporary workarounds we can potentially apply in Nixpkgs for now. The rest will require some distributed effort. Change-Id: I6a6a69648f74d85c6fca86cc52f38fd957e4f9ad --- lix/libstore/build/local-derivation-goal.cc | 6 +-- lix/libstore/ssh.cc | 2 +- lix/libstore/temporary-dir.cc | 5 +-- lix/libstore/temporary-dir.hh | 3 +- lix/libutil/file-system.cc | 50 +++++++-------------- lix/libutil/file-system.hh | 2 +- 6 files changed, 25 insertions(+), 43 deletions(-) diff --git a/lix/libstore/build/local-derivation-goal.cc b/lix/libstore/build/local-derivation-goal.cc index be7c1a08c..3e14e247d 100644 --- a/lix/libstore/build/local-derivation-goal.cc +++ b/lix/libstore/build/local-derivation-goal.cc @@ -447,7 +447,7 @@ try { /* Create a temporary directory where the build will take place. */ tmpDirRoot = - createTempDir(buildDir, "nix-build-" + std::string(drvPath.name()), false, false, 0700); + createTempDir(buildDir, "nix-build-" + std::string(drvPath.name()), 0700); } catch (SysError & e) { /* * Fallback to the global tmpdir and create a safe space there @@ -466,7 +466,7 @@ try { constexpr int toplevelDirMode = 0700; #endif auto nixBuildsTmp = createTempDir( - "", fmt("nix-builds-%s", geteuid()), false, false, toplevelDirMode + "", fmt("nix-builds-%s", geteuid()), toplevelDirMode ); warn( "Failed to use the system-wide build directory '%s', falling back to a temporary " @@ -475,7 +475,7 @@ try { nixBuildsTmp ); tmpDirRoot = createTempDir( - nixBuildsTmp, "nix-build-" + std::string(drvPath.name()), false, false, 0700 + nixBuildsTmp, "nix-build-" + std::string(drvPath.name()), 0700 ); worker.buildDirOverride = nixBuildsTmp; } diff --git a/lix/libstore/ssh.cc b/lix/libstore/ssh.cc index ab3ec5a20..a7b48a330 100644 --- a/lix/libstore/ssh.cc +++ b/lix/libstore/ssh.cc @@ -25,7 +25,7 @@ SSH::SSH(const std::string & host, const std::optional port, const std throw Error("invalid SSH host name '%s'", host); auto state(state_.lock()); - state->tmpDir = std::make_unique(createTempDir("", "nix", true, true, 0700)); + state->tmpDir = std::make_unique(createTempDir("", "nix", 0700)); } void SSH::addCommonSSHOpts(Strings & args) diff --git a/lix/libstore/temporary-dir.cc b/lix/libstore/temporary-dir.cc index 685d5aee7..2e601e34c 100644 --- a/lix/libstore/temporary-dir.cc +++ b/lix/libstore/temporary-dir.cc @@ -5,10 +5,9 @@ namespace nix { -Path createTempDir(const Path & tmpRoot, const Path & prefix, - bool includePid, bool useGlobalCounter, mode_t mode) +Path createTempDir(const Path & tmpRoot, const Path & prefix, mode_t mode) { - return createTempSubdir(tmpRoot.empty() ? defaultTempDir() : tmpRoot, prefix, includePid, useGlobalCounter, mode); + return createTempSubdir(tmpRoot.empty() ? defaultTempDir() : tmpRoot, prefix, mode); } std::pair createTempFile(const Path & prefix) diff --git a/lix/libstore/temporary-dir.hh b/lix/libstore/temporary-dir.hh index f109868c9..9bc4df9c4 100644 --- a/lix/libstore/temporary-dir.hh +++ b/lix/libstore/temporary-dir.hh @@ -8,8 +8,7 @@ namespace nix { /** * Create a temporary directory. */ -Path createTempDir(const Path & tmpRoot = "", const Path & prefix = "nix", - bool includePid = true, bool useGlobalCounter = true, mode_t mode = 0755); +Path createTempDir(const Path & tmpRoot = "", const Path & prefix = "nix", mode_t mode = 0755); /** * Create a temporary file, returning a file handle and its path. diff --git a/lix/libutil/file-system.cc b/lix/libutil/file-system.cc index 3b1d3f676..26ac4fa94 100644 --- a/lix/libutil/file-system.cc +++ b/lix/libutil/file-system.cc @@ -646,44 +646,28 @@ void AutoDelete::reset(const Path & p, bool recursive) { ////////////////////////////////////////////////////////////////////// -static Path tempName(PathView parent, const Path & prefix, bool includePid, - std::atomic & counter) -{ - auto tmpRoot = canonPath(parent, true); - if (includePid) - return fmt("%1%/%2%-%3%-%4%", tmpRoot, prefix, getpid(), counter++); - else - return fmt("%1%/%2%-%3%", tmpRoot, prefix, counter++); -} - Path createTempSubdir(const Path & parent, const Path & prefix, - bool includePid, bool useGlobalCounter, mode_t mode) + mode_t mode) { - static std::atomic globalCounter = 0; - std::atomic localCounter = 0; - auto & counter(useGlobalCounter ? globalCounter : localCounter); - - while (1) { - checkInterrupt(); - Path tmpDir = tempName(parent, prefix, includePid, counter); - if (mkdir(tmpDir.c_str(), mode) == 0) { + checkInterrupt(); + Path tmpDir = makeTempPath(parent + "/", prefix); + if (mkdir(tmpDir.c_str(), mode) == 0) { #if __FreeBSD__ - /* Explicitly set the group of the directory. This is to - work around around problems caused by BSD's group - ownership semantics (directories inherit the group of - the parent). For instance, the group of /tmp on - FreeBSD is "wheel", so all directories created in /tmp - will be owned by "wheel"; but if the user is not in - "wheel", then "tar" will fail to unpack archives that - have the setgid bit set on directories. */ - if (chown(tmpDir.c_str(), (uid_t) -1, getegid()) != 0) - throw SysError("setting group of directory '%1%'", tmpDir); -#endif - return tmpDir; + /* Explicitly set the group of the directory. This is to + work around around problems caused by BSD's group + ownership semantics (directories inherit the group of + the parent). For instance, the group of /tmp on + FreeBSD is "wheel", so all directories created in /tmp + will be owned by "wheel"; but if the user is not in + "wheel", then "tar" will fail to unpack archives that + have the setgid bit set on directories. */ + if (chown(tmpDir.c_str(), (uid_t) -1, getegid()) != 0) { + throw SysError("setting group of directory '%1%'", tmpDir); } - if (errno != EEXIST) - throw SysError("creating directory '%1%'", tmpDir); +#endif + return tmpDir; } + throw SysError("creating directory '%1%'", tmpDir); } Path makeTempPath(const Path & root, const Path & prefix) diff --git a/lix/libutil/file-system.hh b/lix/libutil/file-system.hh index fb8b93a80..85af08fdb 100644 --- a/lix/libutil/file-system.hh +++ b/lix/libutil/file-system.hh @@ -308,7 +308,7 @@ typedef std::unique_ptr AutoCloseDir; * Create a temporary directory in a given parent directory. */ Path createTempSubdir(const Path & parent, const Path & prefix = "nix", - bool includePid = true, bool useGlobalCounter = true, mode_t mode = 0755); + mode_t mode = 0755); /** * Return temporary path constructed by appending to a root path.