Merge pull request #15850 from NixOS/serialise-fixes-32-bit-and-framed-sink-perf

libutil: Fix various 32 bit size_t truncations from uint64_t, fix inefficiencies in serialisation code

Author: Mic92

src/libutil-tests/serialise.cc

@@ -22,4 +22,19 @@ TEST(readNum, negativeValuesSerialiseWellDefined)
     EXPECT_EQ(readNum<uint64_t>(*makeNumSource(int16_t(-1))), std::numeric_limits<uint64_t>::max());
 }
 
+TEST(readPadding, works)
+{
+    for (unsigned i = 0; i < 8; ++i)
+        EXPECT_NO_THROW(readPadding(i, *makeNumSource(0)));
+
+    for (unsigned len = 1; len < 8; ++len) {
+        EXPECT_THROW(readPadding(len, *makeNumSource(~uint64_t(0))), SerialisationError);
+        unsigned padLen = 8 - len;
+        for (unsigned byte = 0; byte < padLen; ++byte) {
+            uint64_t val = uint64_t{1} << (8 * byte);
+            EXPECT_THROW(readPadding(len, *makeNumSource(val)), SerialisationError);
+        }
+    }
+}
+
 } // namespace nix

src/libutil/archive.cc

@@ -146,7 +146,14 @@ static void parseContents(CreateRegularFileSink & sink, Source & source)
     sink.preallocateContents(size);
 
     if (sink.skipContents) {
-        source.skip(alignUp(size, 8));
+        uint64_t left = alignUp(size, 8);
+        /* Source::skip takes a size_t, which might be narrower on 32 bit systems, so
+           be careful around truncations. */
+        while (left) {
+            size_t toSkip = std::min<uint64_t>(left, std::numeric_limits<size_t>::max());
+            source.skip(toSkip);
+            left -= toSkip;
+        }
         return;
     }
 

src/libutil/fs-sink.cc

@@ -267,6 +267,9 @@ void RestoreRegularFile::preallocateContents(uint64_t len)
 
 #if HAVE_POSIX_FALLOCATE
     if (len) {
+        if (len > std::numeric_limits<off_t>::max())
+            throw Error("cannot preallocate contents for a file because it's too large");
+
         errno = posix_fallocate(fd.get(), 0, len);
         /* Note that EINVAL may indicate that the underlying
            filesystem doesn't support preallocation (e.g. on

src/libutil/include/nix/util/serialise.hh

@@ -567,6 +567,10 @@ inline uint64_t readLongLong(Source & source)
     return readNum<uint64_t>(source);
 }
 
+/**
+ * Read padding bytes to align `len` up to a multiple of 8. Throws
+ * SerialisationError if any padding byte is non-zero.
+ */
 void readPadding(size_t len, Source & source);
 size_t readString(char * buf, size_t max, Source & source);
 std::string readString(Source & source, size_t max = std::numeric_limits<size_t>::max());
@@ -649,8 +653,8 @@ struct FramedSource : Source
                     auto n = readInt(from);
                     if (!n)
                         break;
-                    std::vector<char> data(n);
-                    from(data.data(), n);
+                    pending.resize(n);
+                    from(pending.data(), n);
                 }
             }
         } catch (...) {
@@ -669,11 +673,12 @@ struct FramedSource : Source
                 eof = true;
                 return 0;
             }
-            pending = std::vector<char>(len);
+            pending.resize(len);
             pos = 0;
             from(pending.data(), len);
         }
 
+        assert(pending.size() > pos); /* Sanity check. */
         auto n = std::min(len, pending.size() - pos);
         memcpy(data, pending.data() + pos, n);
         pos += n;

src/libutil/memory-source-accessor.cc

@@ -234,6 +234,8 @@ void CreateMemoryRegularFile::isExecutable()
 
 void CreateMemoryRegularFile::preallocateContents(uint64_t len)
 {
+    if (len > std::numeric_limits<decltype(regularFile.contents)::size_type>::max())
+        throw Error("cannot preallocate contents for a file that is too large to fit in memory");
     regularFile.contents.reserve(len);
 }
 

src/libutil/serialise.cc

@@ -508,12 +508,11 @@ Sink & operator<<(Sink & sink, const Error & ex)
 void readPadding(size_t len, Source & source)
 {
     if (len % 8) {
-        char zero[8];
+        uint64_t zero = 0;
         size_t n = 8 - (len % 8);
-        source(zero, n);
-        for (unsigned int i = 0; i < n; i++)
-            if (zero[i])
-                throw SerialisationError("non-zero padding");
+        source(reinterpret_cast<char *>(&zero), n);
+        if (zero)
+            throw SerialisationError("non-zero padding");
     }
 }