diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index f1cf3ee03..c0c808e0a 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -248,7 +248,7 @@ LocalStore::LocalStore( else if (curSchema == 0) { /* new store */ curSchema = nixSchemaVersion; openDB(*state, true); - writeFile(schemaPath, fmt("%1%", curSchema), 0666, true); + writeFile(schemaPath, fmt("%1%", curSchema), 0666, FsSync::Yes); } else if (curSchema < nixSchemaVersion) { @@ -299,7 +299,7 @@ LocalStore::LocalStore( txn.commit(); } - writeFile(schemaPath, fmt("%1%", nixSchemaVersion), 0666, true); + writeFile(schemaPath, fmt("%1%", nixSchemaVersion), 0666, FsSync::Yes); lockFile(globalLock.get(), ltRead, true); } diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index b058aa5ad..82818151c 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -559,7 +559,14 @@ void LocalDerivationGoal::startBuilder() } else { tmpDir = topTmpDir; } - chownToBuilder(tmpDir); + + /* The TOCTOU between the previous mkdir call and this open call is unavoidable due to + POSIX semantics.*/ + tmpDirFd = AutoCloseFD{open(tmpDir.c_str(), O_RDONLY | O_NOFOLLOW | O_DIRECTORY)}; + if (!tmpDirFd) + throw SysError("failed to open the build temporary directory descriptor '%1%'", tmpDir); + + chownToBuilder(tmpDirFd.get(), tmpDir); for (auto & [outputName, status] : initialOutputs) { /* Set scratch path we'll actually use during the build. @@ -1157,9 +1164,7 @@ void LocalDerivationGoal::initTmpDir() } else { auto hash = hashString(HashAlgorithm::SHA256, i.first); std::string fn = ".attr-" + hash.to_string(HashFormat::Nix32, false); - Path p = tmpDir + "/" + fn; - writeFile(p, rewriteStrings(i.second, inputRewrites)); - chownToBuilder(p); + writeBuilderFile(fn, rewriteStrings(i.second, inputRewrites)); env[i.first + "Path"] = tmpDirInSandbox + "/" + fn; } } @@ -1264,11 +1269,9 @@ void LocalDerivationGoal::writeStructuredAttrs() auto jsonSh = writeStructuredAttrsShell(json); - writeFile(tmpDir + "/.attrs.sh", rewriteStrings(jsonSh, inputRewrites)); - chownToBuilder(tmpDir + "/.attrs.sh"); + writeBuilderFile(".attrs.sh", rewriteStrings(jsonSh, inputRewrites)); env["NIX_ATTRS_SH_FILE"] = tmpDirInSandbox + "/.attrs.sh"; - writeFile(tmpDir + "/.attrs.json", rewriteStrings(json.dump(), inputRewrites)); - chownToBuilder(tmpDir + "/.attrs.json"); + writeBuilderFile(".attrs.json", rewriteStrings(json.dump(), inputRewrites)); env["NIX_ATTRS_JSON_FILE"] = tmpDirInSandbox + "/.attrs.json"; } } @@ -1779,6 +1782,24 @@ void setupSeccomp() #endif } +void LocalDerivationGoal::chownToBuilder(int fd, const Path & path) +{ + if (!buildUser) return; + if (fchown(fd, buildUser->getUID(), buildUser->getGID()) == -1) + throw SysError("cannot change ownership of file '%1%'", path); +} + +void LocalDerivationGoal::writeBuilderFile( + const std::string & name, + std::string_view contents) +{ + auto path = std::filesystem::path(tmpDir) / name; + AutoCloseFD fd{openat(tmpDirFd.get(), name.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC | O_EXCL | O_NOFOLLOW, 0666)}; + if (!fd) + throw SysError("creating file %s", path); + writeFile(fd, path, contents); + chownToBuilder(fd.get(), path); +} void LocalDerivationGoal::runChild() { @@ -3053,6 +3074,15 @@ void LocalDerivationGoal::checkOutputs(const std::mapisBuiltin()) { diff --git a/src/libstore/unix/build/local-derivation-goal.hh b/src/libstore/unix/build/local-derivation-goal.hh index 1ea247661..74a1e1c50 100644 --- a/src/libstore/unix/build/local-derivation-goal.hh +++ b/src/libstore/unix/build/local-derivation-goal.hh @@ -37,6 +37,11 @@ struct LocalDerivationGoal : public DerivationGoal */ Path topTmpDir; + /** + * The file descriptor of the temporary directory. + */ + AutoCloseFD tmpDirFd; + /** * The path of the temporary directory in the sandbox. */ @@ -244,9 +249,24 @@ struct LocalDerivationGoal : public DerivationGoal /** * Make a file owned by the builder. + * + * SAFETY: this function is prone to TOCTOU as it receives a path and not a descriptor. + * It's only safe to call in a child of a directory only visible to the owner. */ void chownToBuilder(const Path & path); + /** + * Make a file owned by the builder addressed by its file descriptor. + */ + void chownToBuilder(int fd, const Path & path); + + /** + * Create a file in `tmpDir` owned by the builder. + */ + void writeBuilderFile( + const std::string & name, + std::string_view contents); + int getChildStatus() override; /** diff --git a/src/libutil/file-content-address.cc b/src/libutil/file-content-address.cc index 69301d9c8..2b6839346 100644 --- a/src/libutil/file-content-address.cc +++ b/src/libutil/file-content-address.cc @@ -93,7 +93,7 @@ void restorePath( { switch (method) { case FileSerialisationMethod::Flat: - writeFile(path, source, 0666, startFsync); + writeFile(path, source, 0666, startFsync ? FsSync::Yes : FsSync::No); break; case FileSerialisationMethod::NixArchive: restorePath(path, source, startFsync); diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index a609aa083..2e9abf8d9 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -268,7 +268,7 @@ void readFile(const Path & path, Sink & sink) } -void writeFile(const Path & path, std::string_view s, mode_t mode, bool sync) +void writeFile(const Path & path, std::string_view s, mode_t mode, FsSync sync) { AutoCloseFD fd = toDescriptor(open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT // TODO @@ -278,22 +278,29 @@ void writeFile(const Path & path, std::string_view s, mode_t mode, bool sync) , mode)); if (!fd) throw SysError("opening file '%1%'", path); - try { - writeFull(fd.get(), s); - } catch (Error & e) { - e.addTrace({}, "writing file '%1%'", path); - throw; - } - if (sync) - fd.fsync(); - // Explicitly close to make sure exceptions are propagated. + + writeFile(fd, path, s, mode, sync); + + /* Close explicitly to propagate the exceptions. */ fd.close(); - if (sync) - syncParent(path); } +void writeFile(AutoCloseFD & fd, const Path & origPath, std::string_view s, mode_t mode, FsSync sync) +{ + assert(fd); + try { + writeFull(fd.get(), s); -void writeFile(const Path & path, Source & source, mode_t mode, bool sync) + if (sync == FsSync::Yes) + fd.fsync(); + + } catch (Error & e) { + e.addTrace({}, "writing file '%1%'", origPath); + throw; + } +} + +void writeFile(const Path & path, Source & source, mode_t mode, FsSync sync) { AutoCloseFD fd = toDescriptor(open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT // TODO @@ -317,11 +324,11 @@ void writeFile(const Path & path, Source & source, mode_t mode, bool sync) e.addTrace({}, "writing file '%1%'", path); throw; } - if (sync) + if (sync == FsSync::Yes) fd.fsync(); // Explicitly close to make sure exceptions are propagated. fd.close(); - if (sync) + if (sync == FsSync::Yes) syncParent(path); } @@ -384,7 +391,8 @@ static void _deletePath(Descriptor parentfd, const fs::path & path, uint64_t & b #ifndef _WIN32 checkInterrupt(); - std::string name(baseNameOf(path.native())); + std::string name(path.filename()); + assert(name != "." && name != ".." && !name.empty()); struct stat st; if (fstatat(parentfd, name.c_str(), &st, @@ -425,7 +433,7 @@ static void _deletePath(Descriptor parentfd, const fs::path & path, uint64_t & b throw SysError("chmod %1%", path); } - int fd = openat(parentfd, path.c_str(), O_RDONLY); + int fd = openat(parentfd, name.c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW); if (fd == -1) throw SysError("opening directory %1%", path); AutoCloseDir dir(fdopendir(fd)); @@ -437,7 +445,7 @@ static void _deletePath(Descriptor parentfd, const fs::path & path, uint64_t & b checkInterrupt(); std::string childName = dirent->d_name; if (childName == "." || childName == "..") continue; - _deletePath(dirfd(dir.get()), path + "/" + childName, bytesFreed); + _deletePath(dirfd(dir.get()), path / childName, bytesFreed); } if (errno) throw SysError("reading directory %1%", path); } @@ -455,14 +463,13 @@ static void _deletePath(Descriptor parentfd, const fs::path & path, uint64_t & b static void _deletePath(const fs::path & path, uint64_t & bytesFreed) { - Path dir = dirOf(path.string()); - if (dir == "") - dir = "/"; + assert(path.is_absolute()); + assert(path.parent_path() != path); - AutoCloseFD dirfd = toDescriptor(open(dir.c_str(), O_RDONLY)); + AutoCloseFD dirfd = toDescriptor(open(path.parent_path().string().c_str(), O_RDONLY)); if (!dirfd) { if (errno == ENOENT) return; - throw SysError("opening directory '%1%'", path); + throw SysError("opening directory %s", path.parent_path()); } _deletePath(dirfd.get(), path, bytesFreed); diff --git a/src/libutil/file-system.hh b/src/libutil/file-system.hh index 61bfe6b30..0d9b65592 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -193,21 +193,27 @@ std::string readFile(const Path & path); std::string readFile(const std::filesystem::path & path); void readFile(const Path & path, Sink & sink); +enum struct FsSync { Yes, No }; + /** * Write a string to a file. */ -void writeFile(const Path & path, std::string_view s, mode_t mode = 0666, bool sync = false); -static inline void writeFile(const std::filesystem::path & path, std::string_view s, mode_t mode = 0666, bool sync = false) +void writeFile(const Path & path, std::string_view s, mode_t mode = 0666, FsSync sync = FsSync::No); + +static inline void writeFile(const std::filesystem::path & path, std::string_view s, mode_t mode = 0666, FsSync sync = FsSync::No) { return writeFile(path.string(), s, mode, sync); } -void writeFile(const Path & path, Source & source, mode_t mode = 0666, bool sync = false); -static inline void writeFile(const std::filesystem::path & path, Source & source, mode_t mode = 0666, bool sync = false) +void writeFile(const Path & path, Source & source, mode_t mode = 0666, FsSync sync = FsSync::No); + +static inline void writeFile(const std::filesystem::path & path, Source & source, mode_t mode = 0666, FsSync sync = FsSync::No) { return writeFile(path.string(), source, mode, sync); } +void writeFile(AutoCloseFD & fd, const Path & origPath, std::string_view s, mode_t mode = 0666, FsSync sync = FsSync::No); + /** * Flush a path's parent directory to disk. */