From 9b363e1e5c3c2f9515e8ed0a593dad7e3181e91d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 19 Feb 2026 17:21:12 -0500 Subject: [PATCH 1/7] libutil-tests: Use FS_ROOT macros consistently in file-system tests Add FS_ROOT_NO_TRAILING_SLASH macro and update isInDir and isDirOrInDir tests to use FS_ROOT and FS_SEP macros for cross-platform compatibility. --- src/libutil-tests/file-system.cc | 36 +++++++++++++++++--------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/src/libutil-tests/file-system.cc b/src/libutil-tests/file-system.cc index 6bfcb7d0c..f92aa0492 100644 --- a/src/libutil-tests/file-system.cc +++ b/src/libutil-tests/file-system.cc @@ -16,10 +16,12 @@ using namespace std::string_view_literals; #ifdef _WIN32 # define FS_SEP L"\\" -# define FS_ROOT L"C:" FS_SEP // Need a mounted one, C drive is likely +# define FS_ROOT_NO_TRAILING_SLASH L"C:" // Need a mounted one, C drive is likely +# define FS_ROOT FS_ROOT_NO_TRAILING_SLASH FS_SEP #else # define FS_SEP "/" -# define FS_ROOT FS_SEP +# define FS_ROOT_NO_TRAILING_SLASH FS_SEP +# define FS_ROOT FS_ROOT_NO_TRAILING_SLASH #endif #ifndef PATH_MAX @@ -44,7 +46,7 @@ TEST(absPath, doesntChangeRoot) { auto p = absPath(std::filesystem::path{FS_ROOT}); - ASSERT_EQ(p, FS_ROOT); + ASSERT_EQ(p, FS_ROOT_NO_TRAILING_SLASH); } TEST(absPath, turnsEmptyPathIntoCWD) @@ -196,42 +198,42 @@ TEST(baseNameOf, absoluteNothingSlashNothing) TEST(isInDir, trivialCase) { - EXPECT_TRUE(isInDir("/foo/bar", "/foo")); + EXPECT_TRUE(isInDir(FS_ROOT "foo" FS_SEP "bar", FS_ROOT "foo")); } TEST(isInDir, notInDir) { - EXPECT_FALSE(isInDir("/zes/foo/bar", "/foo")); + EXPECT_FALSE(isInDir(FS_ROOT "zes" FS_SEP "foo" FS_SEP "bar", FS_ROOT "foo")); } TEST(isInDir, emptyDir) { - EXPECT_FALSE(isInDir("/zes/foo/bar", "")); + EXPECT_FALSE(isInDir(FS_ROOT "zes" FS_SEP "foo" FS_SEP "bar", "")); } TEST(isInDir, hiddenSubdirectory) { - EXPECT_TRUE(isInDir("/foo/.ssh", "/foo")); + EXPECT_TRUE(isInDir(FS_ROOT "foo" FS_SEP ".ssh", FS_ROOT "foo")); } TEST(isInDir, ellipsisEntry) { - EXPECT_TRUE(isInDir("/foo/...", "/foo")); + EXPECT_TRUE(isInDir(FS_ROOT "foo" FS_SEP "...", FS_ROOT "foo")); } TEST(isInDir, sameDir) { - EXPECT_FALSE(isInDir("/foo", "/foo")); + EXPECT_FALSE(isInDir(FS_ROOT "foo", FS_ROOT "foo")); } TEST(isInDir, sameDirDot) { - EXPECT_FALSE(isInDir("/foo/.", "/foo")); + EXPECT_FALSE(isInDir(FS_ROOT "foo" FS_SEP ".", FS_ROOT "foo")); } TEST(isInDir, dotDotPrefix) { - EXPECT_FALSE(isInDir("/foo/../bar", "/foo")); + EXPECT_FALSE(isInDir(FS_ROOT "foo" FS_SEP ".." FS_SEP "bar", FS_ROOT "foo")); } /* ---------------------------------------------------------------------------- @@ -240,8 +242,8 @@ TEST(isInDir, dotDotPrefix) TEST(isDirOrInDir, trueForSameDirectory) { - ASSERT_EQ(isDirOrInDir("/nix", "/nix"), true); - ASSERT_EQ(isDirOrInDir("/", "/"), true); + ASSERT_EQ(isDirOrInDir(FS_ROOT "nix", FS_ROOT "nix"), true); + ASSERT_EQ(isDirOrInDir(FS_ROOT, FS_ROOT), true); } TEST(isDirOrInDir, trueForEmptyPaths) @@ -251,17 +253,17 @@ TEST(isDirOrInDir, trueForEmptyPaths) TEST(isDirOrInDir, falseForDisjunctPaths) { - ASSERT_EQ(isDirOrInDir("/foo", "/bar"), false); + ASSERT_EQ(isDirOrInDir(FS_ROOT "foo", FS_ROOT "bar"), false); } TEST(isDirOrInDir, relativePaths) { - ASSERT_EQ(isDirOrInDir("/foo/..", "/foo"), false); + ASSERT_EQ(isDirOrInDir(FS_ROOT "foo" FS_SEP "..", FS_ROOT "foo"), false); } TEST(isDirOrInDir, relativePathsTwice) { - ASSERT_EQ(isDirOrInDir("/foo/..", "/foo/."), false); + ASSERT_EQ(isDirOrInDir(FS_ROOT "foo" FS_SEP "..", FS_ROOT "foo" FS_SEP "."), false); } /* ---------------------------------------------------------------------------- @@ -294,7 +296,7 @@ TEST(makeParentCanonical, noParent) TEST(makeParentCanonical, root) { - ASSERT_EQ(makeParentCanonical("/"), "/"); + ASSERT_EQ(makeParentCanonical(FS_ROOT), FS_ROOT_NO_TRAILING_SLASH); } /* ---------------------------------------------------------------------------- From 0730dcb4a8ab84dc022ebcca9ffe7591bb9be53a Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 19 Feb 2026 17:21:46 -0500 Subject: [PATCH 2/7] Skip `chmodIfNeeded` test on Windows It doesn't do anything on Windows, it appears. --- src/libutil-tests/file-system.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libutil-tests/file-system.cc b/src/libutil-tests/file-system.cc index f92aa0492..eaf62f190 100644 --- a/src/libutil-tests/file-system.cc +++ b/src/libutil-tests/file-system.cc @@ -303,6 +303,9 @@ TEST(makeParentCanonical, root) * chmodIfNeeded * --------------------------------------------------------------------------*/ +#ifndef _WIN32 +// Windows doesn't support Unix-style permission bits - lstat always +// returns the same mode regardless of what chmod sets. TEST(chmodIfNeeded, works) { auto [autoClose_, tmpFile] = nix::createTempFile(); @@ -318,6 +321,7 @@ TEST(chmodIfNeeded, works) } } } +#endif TEST(chmodIfNeeded, nonexistent) { From f0d90d3bdb901a632cb835a64abe0d7d9fb7561c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 19 Feb 2026 17:25:54 -0500 Subject: [PATCH 3/7] Create two more `FSSourceAccessorTest`s Make sure we're testing non-directory roots. --- src/libutil-tests/source-accessor.cc | 30 ++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/libutil-tests/source-accessor.cc b/src/libutil-tests/source-accessor.cc index 9c7e05b82..8f19642a4 100644 --- a/src/libutil-tests/source-accessor.cc +++ b/src/libutil-tests/source-accessor.cc @@ -137,4 +137,34 @@ TEST_F(FSSourceAccessorTest, works) } } +/* ---------------------------------------------------------------------------- + * RestoreSink non-directory at root (no dirFd) + * --------------------------------------------------------------------------*/ + +TEST_F(FSSourceAccessorTest, RestoreSinkRegularFileAtRoot) +{ + auto filePath = tmpDir / "rootfile"; + { + RestoreSink sink(false); + sink.dstPath = filePath; + // No dirFd set - this tests the !dirFd path + sink.createRegularFile(CanonPath::root, [](CreateRegularFileSink & crf) { crf("root content"); }); + } + + EXPECT_THAT(makeFSSourceAccessor(filePath), HasContents(CanonPath::root, "root content")); +} + +TEST_F(FSSourceAccessorTest, RestoreSinkSymlinkAtRoot) +{ + auto linkPath = tmpDir / "rootlink"; + { + RestoreSink sink(false); + sink.dstPath = linkPath; + // No dirFd set - this tests the !dirFd path + sink.createSymlink(CanonPath::root, "symlink_target"); + } + + EXPECT_THAT(makeFSSourceAccessor(linkPath), HasSymlink(CanonPath::root, "symlink_target")); +} + } // namespace nix From 204618c9d84f70c597fd35a4d78d9988c09e37a5 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 19 Feb 2026 17:26:38 -0500 Subject: [PATCH 4/7] Misc unit test improvements --- src/libutil-tests/file-system-at.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libutil-tests/file-system-at.cc b/src/libutil-tests/file-system-at.cc index e60392693..10e633eba 100644 --- a/src/libutil-tests/file-system-at.cc +++ b/src/libutil-tests/file-system-at.cc @@ -84,14 +84,18 @@ TEST(openFileEnsureBeneathNoSymlinks, works) dirSink.createDirectory(CanonPath("d")); dirSink.createSymlink(CanonPath("c"), "./d"); }); +#ifdef _WIN32 + EXPECT_THROW(sink.createDirectory(CanonPath("a/b/c/e")), SymlinkNotAllowed); +#else // FIXME: This still follows symlinks on Unix (incorrectly succeeds) sink.createDirectory(CanonPath("a/b/c/e")); +#endif // Test that symlinks in intermediate path are detected during nested operations - ASSERT_THROW( + EXPECT_THROW( sink.createDirectory( CanonPath("a/b/c/f"), [](FileSystemObjectSink & dirSink, const CanonPath & relPath) {}), SymlinkNotAllowed); - ASSERT_THROW( + EXPECT_THROW( sink.createRegularFile( CanonPath("a/b/c/regular"), [](CreateRegularFileSink & crf) { crf("some contents"); }), SymlinkNotAllowed); From 31d87afc5a08c67ecc6d755c98ee991940459a35 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 19 Feb 2026 19:08:31 -0500 Subject: [PATCH 5/7] `openFileEnsureBeneathNoSymlinks` now returns `AutoCloseFD` This indicates that they are returning an owned handle (i.e. the caller should --- and will, thanks to RAII --- close it). `ntOpenAt` and friends (internal) also use `AutoCloseFD` for the same reason. --- src/libutil-tests/file-system-at.cc | 16 ++++++++-------- src/libutil/fs-sink.cc | 2 +- src/libutil/include/nix/util/file-system-at.hh | 2 +- src/libutil/unix/file-system-at.cc | 12 ++++++------ src/libutil/windows/file-system-at.cc | 17 ++++++++--------- 5 files changed, 24 insertions(+), 25 deletions(-) diff --git a/src/libutil-tests/file-system-at.cc b/src/libutil-tests/file-system-at.cc index 10e633eba..fc387fe80 100644 --- a/src/libutil-tests/file-system-at.cc +++ b/src/libutil-tests/file-system-at.cc @@ -104,7 +104,7 @@ TEST(openFileEnsureBeneathNoSymlinks, works) AutoCloseFD dirFd = openDirectory(tmpDir); // Helper to open files with platform-specific arguments - auto openRead = [&](std::string_view path) -> Descriptor { + auto openRead = [&](std::string_view path) -> AutoCloseFD { return openFileEnsureBeneathNoSymlinks( dirFd.get(), CanonPath(path), @@ -118,7 +118,7 @@ TEST(openFileEnsureBeneathNoSymlinks, works) ); }; - auto openReadDir = [&](std::string_view path) -> Descriptor { + auto openReadDir = [&](std::string_view path) -> AutoCloseFD { return openFileEnsureBeneathNoSymlinks( dirFd.get(), CanonPath(path), @@ -132,7 +132,7 @@ TEST(openFileEnsureBeneathNoSymlinks, works) ); }; - auto openCreateExclusive = [&](std::string_view path) -> Descriptor { + auto openCreateExclusive = [&](std::string_view path) -> AutoCloseFD { return openFileEnsureBeneathNoSymlinks( dirFd.get(), CanonPath(path), @@ -158,19 +158,19 @@ TEST(openFileEnsureBeneathNoSymlinks, works) #if !defined(_WIN32) && !defined(__CYGWIN__) // This returns ELOOP on cygwin when O_NOFOLLOW is used - EXPECT_EQ(openCreateExclusive("a/broken_symlink"), INVALID_DESCRIPTOR); + EXPECT_FALSE(openCreateExclusive("a/broken_symlink")); /* Sanity check, no symlink shenanigans and behaves the same as regular openat with O_EXCL | O_CREAT. */ EXPECT_EQ(errno, EEXIST); #endif EXPECT_THROW(openCreateExclusive("a/absolute_symlink/broken_symlink"), SymlinkNotAllowed); // Test invalid paths - EXPECT_EQ(openRead("c/d/regular/a"), INVALID_DESCRIPTOR); - EXPECT_EQ(openReadDir("c/d/regular"), INVALID_DESCRIPTOR); + EXPECT_FALSE(openRead("c/d/regular/a")); + EXPECT_FALSE(openReadDir("c/d/regular")); // Test valid paths work - EXPECT_TRUE(AutoCloseFD{openRead("c/d/regular")}); - EXPECT_TRUE(AutoCloseFD{openCreateExclusive("a/regular")}); + EXPECT_TRUE(openRead("c/d/regular")); + EXPECT_TRUE(openCreateExclusive("a/regular")); } } // namespace nix diff --git a/src/libutil/fs-sink.cc b/src/libutil/fs-sink.cc index a7e55aea8..1bfeb1b35 100644 --- a/src/libutil/fs-sink.cc +++ b/src/libutil/fs-sink.cc @@ -181,7 +181,7 @@ void RestoreSink::createRegularFile(const CanonPath & path, std::function Date: Thu, 19 Feb 2026 19:23:14 -0500 Subject: [PATCH 6/7] libutil: refactor `AutoCloseFD::fsync` into standalone `syncDescriptor` `AutoCloseFD::fsync()` contained platform-specific logic behind CPP guards. This extracts it into a free function `syncDescriptor(Descriptor)` with separate Unix and Windows implementations, and makes `AutoCloseFD::fsync()` an inline wrapper that delegates to it. The Windows implementation uses `FlushFileBuffers` (returns `BOOL`, true on success) while Unix uses `::fsync` (or `F_FULLFSYNC` on macOS), so splitting them avoids conflating the two calling conventions. --- src/libutil/file-descriptor.cc | 18 ------------------ .../include/nix/util/file-descriptor.hh | 11 ++++++++++- src/libutil/unix/file-descriptor.cc | 13 +++++++++++++ src/libutil/windows/file-descriptor.cc | 6 ++++++ 4 files changed, 29 insertions(+), 19 deletions(-) diff --git a/src/libutil/file-descriptor.cc b/src/libutil/file-descriptor.cc index d4277b778..f16e832bb 100644 --- a/src/libutil/file-descriptor.cc +++ b/src/libutil/file-descriptor.cc @@ -184,24 +184,6 @@ void AutoCloseFD::close() } } -void AutoCloseFD::fsync() const -{ - if (fd != INVALID_DESCRIPTOR) { - int result; - result = -#ifdef _WIN32 - ::FlushFileBuffers(fd) -#elif defined(__APPLE__) - ::fcntl(fd, F_FULLFSYNC) -#else - ::fsync(fd) -#endif - ; - if (result == -1) - throw NativeSysError("fsync file descriptor %1%", fd); - } -} - void AutoCloseFD::startFsync() const { #ifdef __linux__ diff --git a/src/libutil/include/nix/util/file-descriptor.hh b/src/libutil/include/nix/util/file-descriptor.hh index 6283dbbe8..e92878015 100644 --- a/src/libutil/include/nix/util/file-descriptor.hh +++ b/src/libutil/include/nix/util/file-descriptor.hh @@ -133,6 +133,11 @@ std::string readLine(Descriptor fd, bool eofOk = false, char terminator = '\n'); */ void writeLine(Descriptor fd, std::string s); +/** + * Perform a blocking fsync operation on a file descriptor. + */ +void syncDescriptor(Descriptor fd); + /** * Options for draining a file descriptor to a sink. */ @@ -253,7 +258,11 @@ public: /** * Perform a blocking fsync operation. */ - void fsync() const; + void fsync() const + { + if (fd != INVALID_DESCRIPTOR) + nix::syncDescriptor(fd); + } /** * Asynchronously flush to disk without blocking, if available on diff --git a/src/libutil/unix/file-descriptor.cc b/src/libutil/unix/file-descriptor.cc index 86177d01b..643268aeb 100644 --- a/src/libutil/unix/file-descriptor.cc +++ b/src/libutil/unix/file-descriptor.cc @@ -207,4 +207,17 @@ void unix::closeOnExec(int fd) throw SysError("setting close-on-exec flag"); } +void syncDescriptor(Descriptor fd) +{ + int result = +#if defined(__APPLE__) + ::fcntl(fd, F_FULLFSYNC) +#else + ::fsync(fd) +#endif + ; + if (result == -1) + throw NativeSysError("fsync file descriptor %1%", fd); +} + } // namespace nix diff --git a/src/libutil/windows/file-descriptor.cc b/src/libutil/windows/file-descriptor.cc index 44a13cc91..66af6d07d 100644 --- a/src/libutil/windows/file-descriptor.cc +++ b/src/libutil/windows/file-descriptor.cc @@ -148,4 +148,10 @@ off_t lseek(HANDLE h, off_t offset, int whence) return newPos.QuadPart; } +void syncDescriptor(Descriptor fd) +{ + if (!::FlushFileBuffers(fd)) + throw WinError("FlushFileBuffers file descriptor %1%", fd); +} + } // namespace nix From ae4e229c9f95a8285f8059e5c11819d64524a63e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 19 Feb 2026 19:30:55 -0500 Subject: [PATCH 7/7] Change `writeFile(AutoCloseFD &, ...)` to take a `Descriptor` The new signature is: writeFile(Descriptor fd, std::string_view s, FsSync sync = FsSync::No, const Path * origPath = nullptr) This uses `descriptorToPath` if `origPath` is not provided. --- src/libstore/unix/build/derivation-builder.cc | 2 +- src/libutil/file-system.cc | 12 ++++++------ src/libutil/include/nix/util/file-system.hh | 3 +-- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 504063da0..903fe3e96 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -1269,7 +1269,7 @@ void DerivationBuilderImpl::writeBuilderFile(const std::string & name, std::stri 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", PathFmt(path)); - writeFile(fd, path, contents); + writeFile(fd.get(), contents); chownToBuilder(fd.get(), path); } diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index f4ed33ffe..2940ac44e 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -340,23 +340,23 @@ void writeFile(const Path & path, std::string_view s, mode_t mode, FsSync sync) if (!fd) throw SysError("opening file '%1%'", path); - writeFile(fd, path, s, mode, sync); + writeFile(fd.get(), s, sync, &path); /* Close explicitly to propagate the exceptions. */ fd.close(); } -void writeFile(AutoCloseFD & fd, const Path & origPath, std::string_view s, mode_t mode, FsSync sync) +void writeFile(Descriptor fd, std::string_view s, FsSync sync, const Path * origPath) { - assert(fd); + assert(fd != INVALID_DESCRIPTOR); try { - writeFull(fd.get(), s); + writeFull(fd, s); if (sync == FsSync::Yes) - fd.fsync(); + syncDescriptor(fd); } catch (Error & e) { - e.addTrace({}, "writing file '%1%'", origPath); + e.addTrace({}, "writing file '%1%'", origPath ? *origPath : descriptorToPath(fd).string()); throw; } } diff --git a/src/libutil/include/nix/util/file-system.hh b/src/libutil/include/nix/util/file-system.hh index 310daa01e..c0966deae 100644 --- a/src/libutil/include/nix/util/file-system.hh +++ b/src/libutil/include/nix/util/file-system.hh @@ -266,8 +266,7 @@ writeFile(const std::filesystem::path & path, Source & source, mode_t mode = 066 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); +void writeFile(Descriptor fd, std::string_view s, FsSync sync = FsSync::No, const Path * origPath = nullptr); /** * Flush a path's parent directory to disk.