From cd326a2aa4a5ad9118a56d2eff459c73b2ec9226 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Mon, 4 Mar 2024 07:36:51 +0100 Subject: [PATCH] Merge pull request #9673 from pennae/drv-parse-opts optimize derivation parsing (cherry picked from commit 3511430902941f0f26dc71313a54bb5096f57305) Change-Id: I00f76dcd464a5811944613731501af504b6e8c29 --- doc/manual/rl-next/drv-string-parse-hang.md | 6 + src/libstore/derivations.cc | 124 ++++++++++++++------ src/libutil/archive.cc | 2 +- src/libutil/serialise.cc | 2 +- 4 files changed, 94 insertions(+), 40 deletions(-) create mode 100644 doc/manual/rl-next/drv-string-parse-hang.md diff --git a/doc/manual/rl-next/drv-string-parse-hang.md b/doc/manual/rl-next/drv-string-parse-hang.md new file mode 100644 index 000000000..1e041d3e9 --- /dev/null +++ b/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. diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 0013193dd..efba17c4b 100644 --- a/src/libstore/derivations.cc +++ b/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" @@ -149,31 +150,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; } @@ -182,15 +230,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(); @@ -204,12 +252,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; } @@ -262,7 +310,7 @@ static DerivationOutput parseDerivationOutput( } static DerivationOutput parseDerivationOutput( - const Store & store, std::istringstream & str, + const Store & store, StringViewStream & str, const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings) { expect(str, ","); const auto pathS = parseString(str); @@ -270,7 +318,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); } /** @@ -292,7 +340,7 @@ enum struct DerivationATermVersion { static DerivedPathMap::ChildNode parseDerivedPathMapNode( const Store & store, - std::istringstream & str, + StringViewStream & str, DerivationATermVersion version) { DerivedPathMap::ChildNode node; @@ -318,7 +366,7 @@ static DerivedPathMap::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, ")"); @@ -344,7 +392,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()) { @@ -355,12 +403,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; @@ -372,7 +420,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)); } @@ -381,28 +429,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, ")"); diff --git a/src/libutil/archive.cc b/src/libutil/archive.cc index 268a798d9..ce10a35ae 100644 --- a/src/libutil/archive.cc +++ b/src/libutil/archive.cc @@ -161,7 +161,7 @@ static void parseContents(ParseSink & sink, Source & source, const Path & path) sink.preallocateContents(size); uint64_t left = size; - std::vector buf(65536); + std::array buf; while (left) { checkInterrupt(); diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index 6539abed1..910a760a9 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -78,7 +78,7 @@ void Source::operator () (char * data, size_t len) void Source::drainInto(Sink & sink) { std::string s; - std::vector buf(8192); + std::array buf; while (true) { size_t n; try {