nar-accessor: Fix thread safety of seekableGetNarBytes, use Sink, reduce memory usage of nix store cat

src/libfetchers/filtering-source-accessor.cc

@@ -10,12 +10,6 @@ std::optional<std::filesystem::path> FilteringSourceAccessor::getPhysicalPath(co
     return next->getPhysicalPath(prefix / path);
 }
 
-std::string FilteringSourceAccessor::readFile(const CanonPath & path)
-{
-    checkAccess(path);
-    return next->readFile(prefix / path);
-}
-
 void FilteringSourceAccessor::readFile(const CanonPath & path, Sink & sink, std::function<void(uint64_t)> sizeCallback)
 {
     checkAccess(path);

src/libfetchers/git-utils.cc

@@ -766,7 +766,7 @@ struct GitSourceAccessor : SourceAccessor
     {
     }
 
-    std::string readBlob(const CanonPath & path, bool symlink)
+    void readBlob(const CanonPath & path, bool symlink, Sink & sink, std::function<void(uint64_t)> sizeCallback)
     {
         auto state(state_.lock());
 
@@ -785,16 +785,22 @@ struct GitSourceAccessor : SourceAccessor
                     e.addTrace({}, "while smudging git-lfs file '%s'", path);
                     throw;
                 }
-                return s.s;
+                sizeCallback(s.s.size());
+                StringSource source{s.s};
+                source.drainInto(sink);
+                return;
             }
         }
 
-        return std::string((const char *) git_blob_rawcontent(blob.get()), git_blob_rawsize(blob.get()));
+        auto view = std::string_view((const char *) git_blob_rawcontent(blob.get()), git_blob_rawsize(blob.get()));
+        sizeCallback(view.size());
+        StringSource source{view};
+        source.drainInto(sink);
     }
 
-    std::string readFile(const CanonPath & path) override
+    void readFile(const CanonPath & path, Sink & sink, std::function<void(uint64_t)> sizeCallback) override
     {
-        return readBlob(path, false);
+        return readBlob(path, false, sink, sizeCallback);
     }
 
     bool pathExists(const CanonPath & path) override
@@ -861,7 +867,9 @@ struct GitSourceAccessor : SourceAccessor
 
     std::string readLink(const CanonPath & path) override
     {
-        return readBlob(path, true);
+        StringSink s;
+        readBlob(path, true, s, [&](uint64_t size) { s.s.reserve(size); });
+        return std::move(s.s);
     }
 
     /**

src/libfetchers/include/nix/fetchers/filtering-source-accessor.hh

@@ -34,7 +34,7 @@ struct FilteringSourceAccessor : SourceAccessor
 
     std::optional<std::filesystem::path> getPhysicalPath(const CanonPath & path) override;
 
-    std::string readFile(const CanonPath & path) override;
+    using SourceAccessor::readFile;
 
     void readFile(const CanonPath & path, Sink & sink, std::function<void(uint64_t)> sizeCallback) override;
 

src/libstore/dummy-store.cc

@@ -80,12 +80,6 @@ class WholeStoreViewAccessor : public SourceAccessor
         subdirs.emplace(baseName, std::move(accessor));
     }
 
-    std::string readFile(const CanonPath & path) override
-    {
-        return callWithAccessorForPath(
-            path, [](SourceAccessor & accessor, const CanonPath & path) { return accessor.readFile(path); });
-    }
-
     void readFile(const CanonPath & path, Sink & sink, std::function<void(uint64_t)> sizeCallback) override
     {
         return callWithAccessorForPath(path, [&](SourceAccessor & accessor, const CanonPath & path) {

src/libstore/include/nix/store/remote-fs-accessor.hh

@@ -45,7 +45,9 @@ public:
 
     DirEntries readDirectory(const CanonPath & path) override;
 
-    std::string readFile(const CanonPath & path) override;
+    void readFile(const CanonPath & path, Sink & sink, std::function<void(uint64_t)> sizeCallback) override;
+
+    using SourceAccessor::readFile;
 
     std::string readLink(const CanonPath & path) override;
 };

src/libstore/remote-fs-accessor.cc

@@ -108,10 +108,10 @@ SourceAccessor::DirEntries RemoteFSAccessor::readDirectory(const CanonPath & pat
     return res.first->readDirectory(res.second);
 }
 
-std::string RemoteFSAccessor::readFile(const CanonPath & path)
+void RemoteFSAccessor::readFile(const CanonPath & path, Sink & sink, std::function<void(uint64_t)> sizeCallback)
 {
     auto res = fetch(path);
-    return res.first->readFile(res.second);
+    res.first->readFile(res.second, sink, sizeCallback);
 }
 
 std::string RemoteFSAccessor::readLink(const CanonPath & path)

src/libutil/include/nix/util/file-descriptor.hh

@@ -67,6 +67,15 @@ static inline int fromDescriptorReadOnly(Descriptor fd)
  */
 std::string readFile(Descriptor fd);
 
+/**
+ * Read \ref nbytes starting at \ref offset from a seekable file into a sink.
+ *
+ * @throws SystemError if fd is not seekable or any operation fails
+ * @throws Interrupted if the operation was interrupted
+ * @throws EndOfFile if an EOF was reached before reading \ref nbytes
+ */
+void copyFdRange(Descriptor fd, off_t offset, size_t nbytes, Sink & sink);
+
 /**
  * Wrappers around read()/write() that read/write exactly the
  * requested number of bytes.

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

@@ -119,7 +119,9 @@ struct MemorySourceAccessor : virtual SourceAccessor
         return root < other.root;
     }
 
-    std::string readFile(const CanonPath & path) override;
+    void readFile(const CanonPath & path, Sink & sink, std::function<void(uint64_t)> sizeCallback) override;
+    using SourceAccessor::readFile;
+
     bool pathExists(const CanonPath & path) override;
     std::optional<Stat> maybeLstat(const CanonPath & path) override;
     DirEntries readDirectory(const CanonPath & path) override;

src/libutil/include/nix/util/nar-accessor.hh

@@ -35,7 +35,7 @@ ref<NarAccessor> makeNarAccessor(Source & source);
  * readFile() method of the accessor to get the contents of files
  * inside the NAR.
  */
-using GetNarBytes = std::function<std::string(uint64_t, uint64_t)>;
+using GetNarBytes = std::function<void(uint64_t, uint64_t, Sink &)>;
 
 /**
  * The canonical GetNarBytes function for a seekable Source.

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

@@ -33,6 +33,8 @@ public:
 
     void readFile(const CanonPath & path, Sink & sink, std::function<void(uint64_t)> sizeCallback) override;
 
+    using SourceAccessor::readFile;
+
     bool pathExists(const CanonPath & path) override;
 
     std::optional<Stat> maybeLstat(const CanonPath & path) override;

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

@@ -62,7 +62,7 @@ struct SourceAccessor : std::enable_shared_from_this<SourceAccessor>
      * targets of symlinks should only occasionally be done, and only
      * with care.
      */
-    virtual std::string readFile(const CanonPath & path);
+    std::string readFile(const CanonPath & path);
 
     /**
      * Write the contents of a file as a sink. `sizeCallback` must be

src/libutil/memory-source-accessor.cc

@@ -53,14 +53,16 @@ MemorySourceAccessor::File * MemorySourceAccessor::open(const CanonPath & path,
     return cur;
 }
 
-std::string MemorySourceAccessor::readFile(const CanonPath & path)
+void MemorySourceAccessor::readFile(const CanonPath & path, Sink & sink, std::function<void(uint64_t)> sizeCallback)
 {
     auto * f = open(path, std::nullopt);
     if (!f)
         throw FileNotFound("file '%s' does not exist", showPath(path));
-    if (auto * r = std::get_if<File::Regular>(&f->raw))
-        return r->contents;
-    else
+    if (auto * r = std::get_if<File::Regular>(&f->raw)) {
+        sizeCallback(r->contents.size());
+        StringSource source{r->contents};
+        source.drainInto(sink);
+    } else
         throw NotARegularFile("file '%s' is not a regular file", showPath(path));
 }
 

src/libutil/mounted-source-accessor.cc

@@ -21,10 +21,10 @@ struct MountedSourceAccessorImpl : MountedSourceAccessor
         // FIXME: return dummy parent directories automatically?
     }
 
-    std::string readFile(const CanonPath & path) override
+    void readFile(const CanonPath & path, Sink & sink, std::function<void(uint64_t)> sizeCallback) override
     {
         auto [accessor, subpath] = resolve(path);
-        return accessor->readFile(subpath);
+        return accessor->readFile(subpath, sink, sizeCallback);
     }
 
     Stat lstat(const CanonPath & path) override

src/libutil/nar-accessor.cc

@@ -1,6 +1,7 @@
 #include "nix/util/nar-accessor.hh"
 #include "nix/util/file-descriptor.hh"
 #include "nix/util/error.hh"
+#include "nix/util/signals.hh"
 
 namespace nix {
 
@@ -20,8 +21,16 @@ struct NarAccessorImpl : NarAccessor
             StringSource source(nar);
             return parseNarListing(source);
         }()}
-        , getNarBytes{
-              [nar = std::move(nar)](uint64_t offset, uint64_t length) { return std::string{nar, offset, length}; }}
+        , getNarBytes{[nar = std::move(nar)](uint64_t offset, uint64_t length, Sink & sink) {
+            if (offset > nar.size() || length > nar.size() - offset)
+                throw Error(
+                    "reading invalid NAR bytes range: requested %1% bytes at offset %2%, but NAR has size %3%",
+                    length,
+                    offset,
+                    nar.size());
+            StringSource source(std::string_view(nar.data() + offset, length));
+            source.drainInto(sink);
+        }}
     {
     }
 
@@ -111,15 +120,16 @@ struct NarAccessorImpl : NarAccessor
         return res;
     }
 
-    std::string readFile(const CanonPath & path) override
+    void readFile(const CanonPath & path, Sink & sink, std::function<void(uint64_t)> sizeCallback) override
     {
         auto & i = get(path);
         auto * reg = std::get_if<NarListing::Regular>(&i.raw);
         if (!reg)
             throw Error("path '%1%' inside NAR file is not a regular file", path);
 
         assert(getNarBytes);
-        return getNarBytes(*reg->contents.narOffset, *reg->contents.fileSize);
+        sizeCallback(reg->contents.fileSize.value());
+        return getNarBytes(reg->contents.narOffset.value(), reg->contents.fileSize.value(), sink);
     }
 
     std::string readLink(const CanonPath & path) override
@@ -159,19 +169,17 @@ GetNarBytes seekableGetNarBytes(const std::filesystem::path & path)
         throw NativeSysError("opening NAR cache file %s", path);
 
     return [inner = seekableGetNarBytes(fd.get()), fd = make_ref<AutoCloseFD>(std::move(fd))](
-               uint64_t offset, uint64_t length) { return inner(offset, length); };
+               uint64_t offset, uint64_t length, Sink & sink) { return inner(offset, length, sink); };
 }
 
 GetNarBytes seekableGetNarBytes(Descriptor fd)
 {
-    return [fd](uint64_t offset, uint64_t length) {
-        if (lseek(fd, offset, SEEK_SET) == -1)
-            throw SysError("seeking in file");
-
-        std::string buf(length, 0);
-        readFull(fd, buf.data(), length);
-
-        return buf;
+    return [fd](uint64_t offset, uint64_t length, Sink & sink) {
+        if (offset >= std::numeric_limits<off_t>::max()) /* Just in case off_t is not 64 bits. */
+            throw Error("can't read %1% NAR bytes from offset %2%: offset too big", length, offset);
+        if (length >= std::numeric_limits<size_t>::max()) /* Just in case size_t is 32 bits. */
+            throw Error("can't read %1% NAR bytes from offset %2%: length is too big", length, offset);
+        copyFdRange(fd, static_cast<off_t>(offset), static_cast<size_t>(length), sink);
     };
 }
 

src/libutil/union-source-accessor.cc

@@ -12,12 +12,14 @@ struct UnionSourceAccessor : SourceAccessor
         displayPrefix.clear();
     }
 
-    std::string readFile(const CanonPath & path) override
+    void readFile(const CanonPath & path, Sink & sink, std::function<void(uint64_t)> sizeCallback) override
     {
         for (auto & accessor : accessors) {
             auto st = accessor->maybeLstat(path);
-            if (st)
-                return accessor->readFile(path);
+            if (st) {
+                accessor->readFile(path, sink, sizeCallback);
+                return;
+            }
         }
         throw FileNotFound("path '%s' does not exist", showPath(path));
     }

src/libutil/unix/file-descriptor.cc

@@ -166,6 +166,29 @@ void drainFD(int fd, Sink & sink, bool block)
     }
 }
 
+void copyFdRange(Descriptor fd, off_t offset, size_t nbytes, Sink & sink)
+{
+    auto left = nbytes;
+    std::array<char, 64 * 1024> buf;
+
+    while (left) {
+        checkInterrupt();
+        auto limit = std::min<decltype(buf)::size_type>(left, buf.size());
+        ssize_t n = pread(fd, buf.data(), limit, offset);
+        if (n == -1) {
+            if (errno == EINTR)
+                continue;
+            throw SysError("pread of %1% bytes at offset %2%", left, offset);
+        }
+        if (n == 0)
+            throw EndOfFile("unexpected end-of-file");
+        assert(static_cast<size_t>(n) <= left);
+        sink(std::string_view(buf.data(), n));
+        offset += n;
+        left -= n;
+    }
+}
+
 //////////////////////////////////////////////////////////////////////
 
 void Pipe::create()

src/libutil/windows/file-descriptor.cc

@@ -99,6 +99,30 @@ void drainFD(HANDLE handle, Sink & sink /*, bool block*/)
     }
 }
 
+void copyFdRange(Descriptor fd, off_t offset, size_t nbytes, Sink & sink)
+{
+    auto left = nbytes;
+    std::array<char, 64 * 1024> buf;
+
+    while (left) {
+        checkInterrupt();
+        auto limit = std::min<decltype(buf)::size_type>(left, buf.size());
+        OVERLAPPED ov = {};
+        ov.Offset = static_cast<DWORD>(offset);
+        if constexpr (sizeof(offset) > 4) /* We don't build with 32 bit off_t, but let's be safe. */
+            ov.OffsetHigh = static_cast<DWORD>(offset >> 32);
+        DWORD n;
+        if (!ReadFile(fd, buf.data(), static_cast<DWORD>(limit), &n, &ov))
+            throw nix::windows::WinError("ReadFile of %1% bytes at offset %2%", left, offset);
+        if (n == 0)
+            throw EndOfFile("unexpected end-of-file");
+        assert(static_cast<size_t>(n) <= left);
+        sink(std::string_view(buf.data(), n));
+        offset += n;
+        left -= n;
+    }
+}
+
 //////////////////////////////////////////////////////////////////////
 
 void Pipe::create()

src/nix/cat.cc

@@ -18,7 +18,9 @@ struct MixCat : virtual Args
             throw Error("path '%1%' is not a regular file", path.abs());
         logger->stop();
 
-        writeFull(getStandardOutput(), accessor->readFile(path));
+        FdSink output{getStandardOutput()};
+        accessor->readFile(path, output);
+        output.flush();
     }
 };