From 946098a3e08ecb4fce03ba9349dc33ab66fa34d7 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 30 Nov 2021 20:53:10 +0000 Subject: [PATCH 1/3] Nix daemon stream old wopAddToStore No more buffering in string. --- src/libstore/daemon.cc | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 9cbe138fc..183edae79 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -433,25 +433,23 @@ static void performOp(TunnelLogger * logger, ref store, hashAlgo = parseHashType(hashAlgoRaw); } - StringSink saved; - TeeSource savedNARSource(from, saved); - RetrieveRegularNARSink savedRegular { saved }; + auto dumpSource = sinkToSource([&](Sink & saved) { + TeeSource savedNARSource(from, saved); + RetrieveRegularNARSink savedRegular { saved }; - if (method == FileIngestionMethod::Recursive) { - /* Get the entire NAR dump from the client and save it to - a string so that we can pass it to - addToStoreFromDump(). */ - ParseSink sink; /* null sink; just parse the NAR */ - parseDump(sink, savedNARSource); - } else - parseDump(savedRegular, from); + if (method == FileIngestionMethod::Recursive) { + /* Get the entire NAR dump from the client and save it to + a string so that we can pass it to + addToStoreFromDump(). */ + ParseSink sink; /* null sink; just parse the NAR */ + parseDump(sink, savedNARSource); + } else + parseDump(savedRegular, from); + if (!savedRegular.regular) throw Error("regular file expected"); + }); logger->startWork(); - if (!savedRegular.regular) throw Error("regular file expected"); - - // FIXME: try to stream directly from `from`. - StringSource dumpSource { *saved.s }; - auto path = store->addToStoreFromDump(dumpSource, baseName, method, hashAlgo); + auto path = store->addToStoreFromDump(*dumpSource, baseName, method, hashAlgo); logger->stopWork(); to << store->printStorePath(path); From 2f916acf2f4ff0c7857af201c7b67bc3e765120a Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 30 Nov 2021 21:02:45 +0000 Subject: [PATCH 2/3] Push wopAddToStore old style stream adapters into smaller scopes This doesn't fix the bug, but makes the code less difficult to read. Also improve the comments, now that it is clear what part is needed in each code path. --- src/libstore/daemon.cc | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 183edae79..e4bdad4f4 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -434,19 +434,26 @@ static void performOp(TunnelLogger * logger, ref store, } auto dumpSource = sinkToSource([&](Sink & saved) { - TeeSource savedNARSource(from, saved); - RetrieveRegularNARSink savedRegular { saved }; - if (method == FileIngestionMethod::Recursive) { - /* Get the entire NAR dump from the client and save it to - a string so that we can pass it to - addToStoreFromDump(). */ + /* We parse the NAR dump through into `saved` unmodified, + so why all this extra work? We still parse the NAR so + that we aren't sending arbitrary data to `saved` + unwittingly`, and we know when the NAR ends so we don't + consume the rest of `from` and can't parse another + command. (We don't trust `addToStoreFromDump` to not + eagerly consume the entire stream it's given, past the + length of the Nar. */ + TeeSource savedNARSource(from, saved); ParseSink sink; /* null sink; just parse the NAR */ parseDump(sink, savedNARSource); - } else + } else { + /* Incrementally parse the NAR file, stripping the + metadata, and streaming the sole file we expect into + `saved`. */ + RetrieveRegularNARSink savedRegular { saved }; parseDump(savedRegular, from); - - if (!savedRegular.regular) throw Error("regular file expected"); + if (!savedRegular.regular) throw Error("regular file expected"); + } }); logger->startWork(); auto path = store->addToStoreFromDump(*dumpSource, baseName, method, hashAlgo); From 2f92f231dec9a00476aaadc60ba9874887d8cdbd Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 30 Nov 2021 21:48:43 +0000 Subject: [PATCH 3/3] Fix #5299 No matter what, we need to resize the buffer to not have any scratch space after we do the `read`. In the end of file case, `got` will be 0 from it's initial value. Before, we forgot to resize in the EOF case with the break. Yes, we know we didn't recieve any data in that case, but we still have the scatch space to undo. Co-Authored-By: Will Fancher --- src/libstore/local-store.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 5b2490472..b70546cbc 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -8,6 +8,7 @@ #include "references.hh" #include "callback.hh" #include "topo-sort.hh" +#include "finally.hh" #include #include @@ -1327,13 +1328,15 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, const string & name, auto want = std::min(chunkSize, settings.narBufferSize - oldSize); dump.resize(oldSize + want); auto got = 0; + Finally cleanup([&]() { + dump.resize(oldSize + got); + }); try { got = source.read(dump.data() + oldSize, want); } catch (EndOfFile &) { inMemory = true; break; } - dump.resize(oldSize + got); } std::unique_ptr delTempDir;