Merge pull request #14562 from NixOS/no-races-posix-source-accessor

libutil: Make PosixSourceAccessor update mtime only when needed

Author: Ericson2314

src/libutil/archive.cc

@@ -103,9 +103,9 @@ void SourceAccessor::dumpPath(const CanonPath & path, Sink & sink, PathFilter &
 
 time_t dumpPathAndGetMtime(const Path & path, Sink & sink, PathFilter & filter)
 {
-    auto path2 = PosixSourceAccessor::createAtRoot(path);
+    auto path2 = PosixSourceAccessor::createAtRoot(path, /*trackLastModified=*/true);
     path2.dumpPath(sink, filter);
-    return path2.accessor.dynamic_pointer_cast<PosixSourceAccessor>()->mtime;
+    return path2.accessor->getLastModified().value();
 }
 
 void dumpPath(const Path & path, Sink & sink, PathFilter & filter)

src/libutil/include/nix/util/posix-source-accessor.hh

@@ -9,7 +9,7 @@ struct SourcePath;
 /**
  * A source accessor that uses the Unix filesystem.
  */
-struct PosixSourceAccessor : virtual SourceAccessor
+class PosixSourceAccessor : virtual public SourceAccessor
 {
     /**
      * Optional root path to prefix all operations into the native file
@@ -18,8 +18,12 @@ struct PosixSourceAccessor : virtual SourceAccessor
      */
     const std::filesystem::path root;
 
+    const bool trackLastModified = false;
+
+public:
+
     PosixSourceAccessor();
-    PosixSourceAccessor(std::filesystem::path && root);
+    PosixSourceAccessor(std::filesystem::path && root, bool trackLastModified = false);
 
     /**
      * The most recent mtime seen by lstat(). This is a hack to
@@ -43,6 +47,9 @@ struct PosixSourceAccessor : virtual SourceAccessor
      * Create a `PosixSourceAccessor` and `SourcePath` corresponding to
      * some native path.
      *
+     * @param Whether the accessor should return a non-null getLastModified.
+     * When true the accessor must be used only by a single thread.
+     *
      * The `PosixSourceAccessor` is rooted as far up the tree as
      * possible, (e.g. on Windows it could scoped to a drive like
      * `C:\`). This allows more `..` parent accessing to work.
@@ -64,7 +71,12 @@ struct PosixSourceAccessor : virtual SourceAccessor
      * and
      * [`std::filesystem::path::relative_path`](https://en.cppreference.com/w/cpp/filesystem/path/relative_path).
      */
-    static SourcePath createAtRoot(const std::filesystem::path & path);
+    static SourcePath createAtRoot(const std::filesystem::path & path, bool trackLastModified = false);
+
+    std::optional<std::time_t> getLastModified() override
+    {
+        return trackLastModified ? std::optional{mtime} : std::nullopt;
+    }
 
 private:
 

src/libutil/posix-source-accessor.cc

@@ -7,8 +7,9 @@
 
 namespace nix {
 
-PosixSourceAccessor::PosixSourceAccessor(std::filesystem::path && argRoot)
+PosixSourceAccessor::PosixSourceAccessor(std::filesystem::path && argRoot, bool trackLastModified)
     : root(std::move(argRoot))
+    , trackLastModified(trackLastModified)
 {
     assert(root.empty() || root.is_absolute());
     displayPrefix = root.string();
@@ -19,11 +20,11 @@ PosixSourceAccessor::PosixSourceAccessor()
 {
 }
 
-SourcePath PosixSourceAccessor::createAtRoot(const std::filesystem::path & path)
+SourcePath PosixSourceAccessor::createAtRoot(const std::filesystem::path & path, bool trackLastModified)
 {
     std::filesystem::path path2 = absPath(path);
     return {
-        make_ref<PosixSourceAccessor>(path2.root_path()),
+        make_ref<PosixSourceAccessor>(path2.root_path(), trackLastModified),
         CanonPath{path2.relative_path().string()},
     };
 }
@@ -114,9 +115,12 @@ std::optional<SourceAccessor::Stat> PosixSourceAccessor::maybeLstat(const CanonP
     auto st = cachedLstat(path);
     if (!st)
         return std::nullopt;
-    // This makes the accessor thread-unsafe, but we only seem to use the actual value in a single threaded context in
-    // `src/libfetchers/path.cc`.
-    mtime = std::max(mtime, st->st_mtime);
+
+    /* The contract is that trackLastModified implies that the caller uses the accessor
+       from a single thread. Thus this is not a CAS loop. */
+    if (trackLastModified)
+        mtime = std::max(mtime, st->st_mtime);
+
     return Stat{
         .type = S_ISREG(st->st_mode)   ? tRegular
                 : S_ISDIR(st->st_mode) ? tDirectory