Merge pull request #9673 from pennae/drv-parse-opts

optimize derivation parsing

doc/manual/rl-next/drv-string-parse-hang.md

@@ -0,0 +1,6 @@
+---
+synopsis: Fix handling of truncated `.drv` files.
+prs: 9673
+---
+
+Previously a `.drv` that was truncated in the middle of a string would case nix to enter an infinite loop, eventually exhausting all memory and crashing.

src/libstore/derivations.cc

@@ -2,6 +2,7 @@
 #include "downstream-placeholder.hh"
 #include "store-api.hh"
 #include "globals.hh"
+#include "types.hh"
 #include "util.hh"
 #include "split.hh"
 #include "common-protocol.hh"
@@ -154,31 +155,78 @@ StorePath writeDerivation(Store & store,
 }
 
 
-/* Read string `s' from stream `str'. */
-static void expect(std::istream & str, std::string_view s)
-{
-    for (auto & c : s) {
-        if (str.get() != c)
-            throw FormatError("expected string '%1%'", s);
+namespace {
+/**
+ * This mimics std::istream to some extent. We use this much smaller implementation
+ * instead of plain istreams because the sentry object overhead is too high.
+ */
+struct StringViewStream {
+    std::string_view remaining;
+
+    int peek() const {
+        return remaining.empty() ? EOF : remaining[0];
     }
+
+    int get() {
+        if (remaining.empty()) return EOF;
+        char c = remaining[0];
+        remaining.remove_prefix(1);
+        return c;
+    }
+};
+
+constexpr struct Escapes {
+    char map[256];
+    constexpr Escapes() {
+        for (int i = 0; i < 256; i++) map[i] = (char) (unsigned char) i;
+        map[(int) (unsigned char) 'n'] = '\n';
+        map[(int) (unsigned char) 'r'] = '\r';
+        map[(int) (unsigned char) 't'] = '\t';
+    }
+    char operator[](char c) const { return map[(unsigned char) c]; }
+} escapes;
+}
+
+
+/* Read string `s' from stream `str'. */
+static void expect(StringViewStream & str, std::string_view s)
+{
+    if (!str.remaining.starts_with(s))
+        throw FormatError("expected string '%1%'", s);
+    str.remaining.remove_prefix(s.size());
 }
 
 
 /* Read a C-style string from stream `str'. */
-static std::string parseString(std::istream & str)
+static BackedStringView parseString(StringViewStream & str)
 {
-    std::string res;
     expect(str, "\"");
-    int c;
-    while ((c = str.get()) != '"')
-        if (c == '\\') {
-            c = str.get();
-            if (c == 'n') res += '\n';
-            else if (c == 'r') res += '\r';
-            else if (c == 't') res += '\t';
-            else res += c;
+    auto c = str.remaining.begin(), end = str.remaining.end();
+    bool escaped = false;
+    for (; c != end && *c != '"'; c++) {
+        if (*c == '\\') {
+            c++;
+            if (c == end)
+                throw FormatError("unterminated string in derivation");
+            escaped = true;
         }
-        else res += c;
+    }
+
+    const auto contentLen = c - str.remaining.begin();
+    const auto content = str.remaining.substr(0, contentLen);
+    str.remaining.remove_prefix(contentLen + 1);
+
+    if (!escaped)
+        return content;
+
+    std::string res;
+    res.reserve(content.size());
+    for (c = content.begin(), end = content.end(); c != end; c++)
+        if (*c == '\\') {
+            c++;
+            res += escapes[*c];
+        }
+        else res += *c;
     return res;
 }
 
@@ -187,15 +235,15 @@ static void validatePath(std::string_view s) {
         throw FormatError("bad path '%1%' in derivation", s);
 }
 
-static Path parsePath(std::istream & str)
+static BackedStringView parsePath(StringViewStream & str)
 {
     auto s = parseString(str);
-    validatePath(s);
+    validatePath(*s);
     return s;
 }
 
 
-static bool endOfList(std::istream & str)
+static bool endOfList(StringViewStream & str)
 {
     if (str.peek() == ',') {
         str.get();
@@ -209,12 +257,12 @@ static bool endOfList(std::istream & str)
 }
 
 
-static StringSet parseStrings(std::istream & str, bool arePaths)
+static StringSet parseStrings(StringViewStream & str, bool arePaths)
 {
     StringSet res;
     expect(str, "[");
     while (!endOfList(str))
-        res.insert(arePaths ? parsePath(str) : parseString(str));
+        res.insert((arePaths ? parsePath(str) : parseString(str)).toOwned());
     return res;
 }
 
@@ -267,7 +315,7 @@ static DerivationOutput parseDerivationOutput(
 }
 
 static DerivationOutput parseDerivationOutput(
-    const StoreDirConfig & store, std::istringstream & str,
+    const StoreDirConfig & store, StringViewStream & str,
     const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings)
 {
     expect(str, ","); const auto pathS = parseString(str);
@@ -275,7 +323,7 @@ static DerivationOutput parseDerivationOutput(
     expect(str, ","); const auto hash = parseString(str);
     expect(str, ")");
 
-    return parseDerivationOutput(store, pathS, hashAlgo, hash, xpSettings);
+    return parseDerivationOutput(store, *pathS, *hashAlgo, *hash, xpSettings);
 }
 
 /**
@@ -297,7 +345,7 @@ enum struct DerivationATermVersion {
 
 static DerivedPathMap<StringSet>::ChildNode parseDerivedPathMapNode(
     const StoreDirConfig & store,
-    std::istringstream & str,
+    StringViewStream & str,
     DerivationATermVersion version)
 {
     DerivedPathMap<StringSet>::ChildNode node;
@@ -323,7 +371,7 @@ static DerivedPathMap<StringSet>::ChildNode parseDerivedPathMapNode(
             expect(str, ",[");
             while (!endOfList(str)) {
                 expect(str, "(");
-                auto outputName = parseString(str);
+                auto outputName = parseString(str).toOwned();
                 expect(str, ",");
                 node.childMap.insert_or_assign(outputName, parseDerivedPathMapNode(store, str, version));
                 expect(str, ")");
@@ -349,7 +397,7 @@ Derivation parseDerivation(
     Derivation drv;
     drv.name = name;
 
-    std::istringstream str(std::move(s));
+    StringViewStream str{s};
     expect(str, "D");
     DerivationATermVersion version;
     switch (str.peek()) {
@@ -360,12 +408,12 @@ Derivation parseDerivation(
     case 'r': {
         expect(str, "rvWithVersion(");
         auto versionS = parseString(str);
-        if (versionS == "xp-dyn-drv") {
+        if (*versionS == "xp-dyn-drv") {
             // Only verison we have so far
             version = DerivationATermVersion::DynamicDerivations;
             xpSettings.require(Xp::DynamicDerivations);
         } else {
-            throw FormatError("Unknown derivation ATerm format version '%s'", versionS);
+            throw FormatError("Unknown derivation ATerm format version '%s'", *versionS);
         }
         expect(str, ",");
         break;
@@ -377,7 +425,7 @@ Derivation parseDerivation(
     /* Parse the list of outputs. */
     expect(str, "[");
     while (!endOfList(str)) {
-        expect(str, "("); std::string id = parseString(str);
+        expect(str, "("); std::string id = parseString(str).toOwned();
         auto output = parseDerivationOutput(store, str, xpSettings);
         drv.outputs.emplace(std::move(id), std::move(output));
     }
@@ -386,28 +434,28 @@ Derivation parseDerivation(
     expect(str, ",[");
     while (!endOfList(str)) {
         expect(str, "(");
-        Path drvPath = parsePath(str);
+        auto drvPath = parsePath(str);
         expect(str, ",");
-        drv.inputDrvs.map.insert_or_assign(store.parseStorePath(drvPath), parseDerivedPathMapNode(store, str, version));
+        drv.inputDrvs.map.insert_or_assign(store.parseStorePath(*drvPath), parseDerivedPathMapNode(store, str, version));
         expect(str, ")");
     }
 
     expect(str, ","); drv.inputSrcs = store.parseStorePathSet(parseStrings(str, true));
-    expect(str, ","); drv.platform = parseString(str);
-    expect(str, ","); drv.builder = parseString(str);
+    expect(str, ","); drv.platform = parseString(str).toOwned();
+    expect(str, ","); drv.builder = parseString(str).toOwned();
 
     /* Parse the builder arguments. */
     expect(str, ",[");
     while (!endOfList(str))
-        drv.args.push_back(parseString(str));
+        drv.args.push_back(parseString(str).toOwned());
 
     /* Parse the environment variables. */
     expect(str, ",[");
     while (!endOfList(str)) {
-        expect(str, "("); auto name = parseString(str);
-        expect(str, ","); auto value = parseString(str);
+        expect(str, "("); auto name = parseString(str).toOwned();
+        expect(str, ","); auto value = parseString(str).toOwned();
         expect(str, ")");
-        drv.env[name] = value;
+        drv.env.insert_or_assign(std::move(name), std::move(value));
     }
 
     expect(str, ")");

src/libutil/archive.cc

@@ -140,7 +140,7 @@ static void parseContents(ParseSink & sink, Source & source, const Path & path)
     sink.preallocateContents(size);
 
     uint64_t left = size;
-    std::vector<char> buf(65536);
+    std::array<char, 65536> buf;
 
     while (left) {
         checkInterrupt();

src/libutil/file-system.cc

@@ -307,7 +307,7 @@ void writeFile(const Path & path, Source & source, mode_t mode, bool sync)
     if (!fd)
         throw SysError("opening file '%1%'", path);
 
-    std::vector<char> buf(64 * 1024);
+    std::array<char, 64 * 1024> buf;
 
     try {
         while (true) {

src/libutil/serialise.cc

@@ -82,7 +82,7 @@ void Source::operator () (std::string_view data)
 void Source::drainInto(Sink & sink)
 {
     std::string s;
-    std::vector<char> buf(8192);
+    std::array<char, 8192> buf;
     while (true) {
         size_t n;
         try {