Merge pull request #15301 from obsidiansystems/file-system-prep

Misc file system fixes, especially for windows

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);
 }
 

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);
@@ -100,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),
@@ -114,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),
@@ -128,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),
@@ -154,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

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,13 +296,16 @@ TEST(makeParentCanonical, noParent)
 
 TEST(makeParentCanonical, root)
 {
-    ASSERT_EQ(makeParentCanonical("/"), "/");
+    ASSERT_EQ(makeParentCanonical(FS_ROOT), FS_ROOT_NO_TRAILING_SLASH);
 }
 
 /* ----------------------------------------------------------------------------
  * 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();
@@ -316,6 +321,7 @@ TEST(chmodIfNeeded, works)
         }
     }
 }
+#endif
 
 TEST(chmodIfNeeded, nonexistent)
 {

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

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__

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;
     }
 }

src/libutil/fs-sink.cc

@@ -181,7 +181,7 @@ void RestoreSink::createRegularFile(const CanonPath & path, std::function<void(C
                component are not followed. */
             constexpr int flags = O_CREAT | O_EXCL | O_WRONLY | O_CLOEXEC;
             if (!dirFd)
-                return ::open(p.c_str(), flags, 0666);
+                return AutoCloseFD{::open(p.c_str(), flags, 0666)};
             return openFileEnsureBeneathNoSymlinks(dirFd.get(), path, flags, 0666);
         }()
 #endif

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

src/libutil/include/nix/util/file-system-at.hh

@@ -54,7 +54,7 @@ OsString readLinkAt(Descriptor dirFd, const CanonPath & path);
  * @throws SymlinkNotAllowed if any path components are symlinks
  * @throws SystemError on other errors
  */
-Descriptor openFileEnsureBeneathNoSymlinks(
+AutoCloseFD openFileEnsureBeneathNoSymlinks(
     Descriptor dirFd,
     const CanonPath & path,
 #ifdef _WIN32

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.

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

src/libutil/unix/file-system-at.cc

@@ -136,7 +136,7 @@ void unix::fchmodatTryNoFollow(Descriptor dirFd, const CanonPath & path, mode_t
     }
 }
 
-static Descriptor
+static AutoCloseFD
 openFileEnsureBeneathNoSymlinksIterative(Descriptor dirFd, const CanonPath & path, int flags, mode_t mode)
 {
     AutoCloseFD parentFd;
@@ -179,19 +179,19 @@ openFileEnsureBeneathNoSymlinksIterative(Descriptor dirFd, const CanonPath & pat
                 throw SymlinkNotAllowed(path2);
             }
 
-            return INVALID_DESCRIPTOR;
+            return AutoCloseFD{};
         }
 
         parentFd = std::move(parentFd2);
     }
 
-    auto res = ::openat(getParentFd(), std::string(path.baseName().value()).c_str(), flags | O_NOFOLLOW, mode);
-    if (res < 0 && errno == ELOOP)
+    AutoCloseFD res = ::openat(getParentFd(), std::string(path.baseName().value()).c_str(), flags | O_NOFOLLOW, mode);
+    if (!res && errno == ELOOP)
         throw SymlinkNotAllowed(path);
     return res;
 }
 
-Descriptor openFileEnsureBeneathNoSymlinks(Descriptor dirFd, const CanonPath & path, int flags, mode_t mode)
+AutoCloseFD openFileEnsureBeneathNoSymlinks(Descriptor dirFd, const CanonPath & path, int flags, mode_t mode)
 {
     assert(!path.rel().starts_with('/')); /* Just in case the invariant is somehow broken. */
     assert(!path.isRoot());
@@ -201,7 +201,7 @@ Descriptor openFileEnsureBeneathNoSymlinks(Descriptor dirFd, const CanonPath & p
     if (maybeFd) {
         if (*maybeFd < 0 && errno == ELOOP)
             throw SymlinkNotAllowed(path);
-        return *maybeFd;
+        return AutoCloseFD{*maybeFd};
     }
 #endif
     return openFileEnsureBeneathNoSymlinksIterative(dirFd, path, flags, mode);

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

src/libutil/windows/file-system-at.cc

@@ -29,7 +29,7 @@ namespace {
  * @param createDisposition FILE_OPEN, FILE_CREATE, etc.
  * @return Handle to the opened file/directory (caller must close)
  */
-HANDLE ntOpenAt(
+AutoCloseFD ntOpenAt(
     Descriptor dirFd,
     std::wstring_view pathComponent,
     ACCESS_MASK desiredAccess,
@@ -73,7 +73,7 @@ HANDLE ntOpenAt(
         throw WinError(
             RtlNtStatusToDosError(status), "opening %s relative to directory handle", PathFmt(pathComponent));
 
-    return h;
+    return AutoCloseFD{h};
 }
 
 /**
@@ -81,9 +81,9 @@ HANDLE ntOpenAt(
  *
  * @param dirFd Directory handle to open relative to
  * @param path Relative path to the symlink
- * @return Handle to the symlink (caller must close)
+ * @return Handle to the symlink
  */
-HANDLE openSymlinkAt(Descriptor dirFd, const CanonPath & path)
+AutoCloseFD openSymlinkAt(Descriptor dirFd, const CanonPath & path)
 {
     assert(!path.isRoot());
     assert(!path.rel().starts_with('/')); /* Just in case the invariant is somehow broken. */
@@ -210,7 +210,7 @@ bool isReparsePoint(HANDLE handle)
 
 } // namespace windows
 
-Descriptor openFileEnsureBeneathNoSymlinks(
+AutoCloseFD openFileEnsureBeneathNoSymlinks(
     Descriptor dirFd, const CanonPath & path, ACCESS_MASK desiredAccess, ULONG createOptions, ULONG createDisposition)
 {
     assert(!path.isRoot());
@@ -233,9 +233,8 @@ Descriptor openFileEnsureBeneathNoSymlinks(
     /* Helper to check if a component is a symlink and throw SymlinkNotAllowed if so */
     auto throwIfSymlink = [&](std::wstring_view component, const CanonPath & pathForError) {
         try {
-            auto testFd =
+            auto testHandle =
                 ntOpenAt(getParentFd(), component, FILE_READ_ATTRIBUTES | SYNCHRONIZE, FILE_OPEN_REPARSE_POINT);
-            AutoCloseFD testHandle(testFd);
             if (isReparsePoint(testHandle.get()))
                 throw SymlinkNotAllowed(pathForError);
         } catch (SymlinkNotAllowed &) {
@@ -278,7 +277,7 @@ Descriptor openFileEnsureBeneathNoSymlinks(
     /* Now open the final component with requested flags */
     std::wstring finalComponent = string_to_os_string(std::string(path.baseName().value()));
 
-    HANDLE finalHandle;
+    AutoCloseFD finalHandle;
     try {
         finalHandle = ntOpenAt(
             getParentFd(),
@@ -295,7 +294,7 @@ Descriptor openFileEnsureBeneathNoSymlinks(
     }
 
     /* Final check: did we accidentally open a symlink? */
-    if (isReparsePoint(finalHandle))
+    if (isReparsePoint(finalHandle.get()))
         throw SymlinkNotAllowed(path);
 
     return finalHandle;