From fb05a6adcfe0362249bd16527a2e44ea2611e5f3 Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Tue, 16 Jun 2020 12:45:36 -0400 Subject: [PATCH 1/8] Eliminate old TeeSink abstraction This was introduced in fa125b9b28bea25a4eeb4d39a71a481563127cb9, and then "reverted" in 1cf480110879ffc8aee94b4b75999da405b71d7c, except that revert left the struct around doing nothing useful. We're removing it all the way now because we want to make a new `TeeSink` complementing the already-exiting `TeeSource`, that is actually a completely different concept as far as the class hierarchy is concerned. --- src/libstore/daemon.cc | 7 ++++--- src/libstore/export-import.cc | 11 ++++++----- src/libutil/archive.hh | 7 ------- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index e370e278c..b2e37c5b5 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -721,9 +721,10 @@ static void performOp(TunnelLogger * logger, ref store, if (GET_PROTOCOL_MINOR(clientVersion) >= 21) source = std::make_unique(from, to); else { - TeeSink tee(from); - parseDump(tee, tee.source); - saved = std::move(*tee.source.data); + TeeSource tee(from); + ParseSink sink; + parseDump(sink, tee); + saved = std::move(*tee.data); source = std::make_unique(saved); } diff --git a/src/libstore/export-import.cc b/src/libstore/export-import.cc index cb9da027d..0e33fb687 100644 --- a/src/libstore/export-import.cc +++ b/src/libstore/export-import.cc @@ -77,8 +77,9 @@ StorePaths Store::importPaths(Source & source, std::shared_ptr acces if (n != 1) throw Error("input doesn't look like something created by 'nix-store --export'"); /* Extract the NAR from the source. */ - TeeSink tee(source); - parseDump(tee, tee.source); + TeeSource tee(source); + ParseSink sink; + parseDump(sink, tee); uint32_t magic = readInt(source); if (magic != exportMagic) @@ -94,15 +95,15 @@ StorePaths Store::importPaths(Source & source, std::shared_ptr acces if (deriver != "") info.deriver = parseStorePath(deriver); - info.narHash = hashString(htSHA256, *tee.source.data); - info.narSize = tee.source.data->size(); + info.narHash = hashString(htSHA256, *tee.data); + info.narSize = tee.data->size(); // Ignore optional legacy signature. if (readInt(source) == 1) readString(source); // Can't use underlying source, which would have been exhausted - auto source = StringSource { *tee.source.data }; + auto source = StringSource { *tee.data }; addToStore(info, source, NoRepair, checkSigs, accessor); res.push_back(info.path); diff --git a/src/libutil/archive.hh b/src/libutil/archive.hh index 768fe2536..32d98a610 100644 --- a/src/libutil/archive.hh +++ b/src/libutil/archive.hh @@ -63,13 +63,6 @@ struct ParseSink virtual void createSymlink(const Path & path, const string & target) { }; }; -struct TeeSink : ParseSink -{ - TeeSource source; - - TeeSink(Source & source) : source(source) { } -}; - void parseDump(ParseSink & sink, Source & source); void restorePath(const Path & path, Source & source); From 289b9b8dcf8dc651c2d245d9328911f7addd1626 Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Tue, 16 Jun 2020 15:14:11 -0400 Subject: [PATCH 2/8] Create a new TeeSink abstraction This is a bit complex because we want to expose extra functionality the wrapped class has. Perhaps there is some inheritancy trickery to do this nicer, but I don't know it, and this is the first thing we tried after a series of attempts that did build. This design is kind of like that of Rust's Writer, Reader, or Iter adapters, which impliment more traits based on what the inner type implements. --- src/libutil/compression.hh | 12 ++++++++++++ src/libutil/serialise.hh | 22 ++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/src/libutil/compression.hh b/src/libutil/compression.hh index dd666a4e1..1bd118b47 100644 --- a/src/libutil/compression.hh +++ b/src/libutil/compression.hh @@ -25,4 +25,16 @@ MakeError(UnknownCompressionMethod, Error); MakeError(CompressionError, Error); +template<> +struct TeeSink> : CompressionSink +{ + MAKE_TEE_SINK(ref); + void finish() override { + orig->finish(); + } + void write(const unsigned char * data, size_t len) override { + return orig->write(data, len); + } +}; + } diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index a04118512..88a6b7ffe 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -181,6 +181,28 @@ struct TeeSource : Source } }; +#define MAKE_TEE_SINK(T) \ + T orig; \ + ref data; \ + TeeSink(T && orig) \ + : orig(std::move(orig)), data(make_ref()) { } \ + void operator () (const unsigned char * data, size_t len) { \ + this->data->append((const char *) data, len); \ + (*this->orig)(data, len); \ + } \ + void operator () (const std::string & s) \ + { \ + *data += s; \ + (*this->orig)(s); \ + } + +template +struct TeeSink : Sink +{ + MAKE_TEE_SINK(T); +}; + + /* A reader that consumes the original Source until 'size'. */ struct SizedSource : Source { From a835c740ca67e063fe47e7c31444b7c1cac0fe81 Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Tue, 16 Jun 2020 15:14:11 -0400 Subject: [PATCH 3/8] Replace `TransferItem::status` with a local variable Everywhere seems to use `getHTTPStatus` now. --- src/libstore/filetransfer.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 531b85af8..9566e0ae9 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -56,7 +56,6 @@ struct curlFileTransfer : public FileTransfer Callback callback; CURL * req = 0; bool active = false; // whether the handle has been added to the multi object - std::string status; unsigned int attempt = 0; @@ -175,6 +174,7 @@ struct curlFileTransfer : public FileTransfer size_t realSize = size * nmemb; std::string line((char *) contents, realSize); printMsg(lvlVomit, format("got header for '%s': %s") % request.uri % trim(line)); + std::string status; if (line.compare(0, 5, "HTTP/") == 0) { // new response starts result.etag = ""; auto ss = tokenizeString>(line, " "); From 004570a377b2df355064d48afc54375690c3cdb0 Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Tue, 16 Jun 2020 15:14:11 -0400 Subject: [PATCH 4/8] Add HTTP responses to FileTransferErrors --- src/libstore/filetransfer.cc | 26 ++++++++++++++++++++------ src/libstore/filetransfer.hh | 5 +++-- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 9566e0ae9..d89f8388f 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -122,7 +122,7 @@ struct curlFileTransfer : public FileTransfer if (requestHeaders) curl_slist_free_all(requestHeaders); try { if (!done) - fail(FileTransferError(Interrupted, "download of '%s' was interrupted", request.uri)); + fail(FileTransferError(Interrupted, nullptr, "download of '%s' was interrupted", request.uri)); } catch (...) { ignoreException(); } @@ -152,8 +152,18 @@ struct curlFileTransfer : public FileTransfer size_t realSize = size * nmemb; result.bodySize += realSize; - if (!decompressionSink) + if (!decompressionSink) { decompressionSink = makeDecompressionSink(encoding, finalSink); + if (! successfulStatuses.count(getHTTPStatus())) { + // In this case we want to construct a TeeSink, to keep + // the response around (which we figure won't be big + // like an actual download should be) to improve error + // messages. + decompressionSink = std::make_shared>>( + ref{ decompressionSink } + ); + } + } (*decompressionSink)((unsigned char *) contents, realSize); @@ -408,16 +418,20 @@ struct curlFileTransfer : public FileTransfer attempt++; + std::shared_ptr response; + if (decompressionSink) + if (auto teeSink = std::dynamic_pointer_cast>>(decompressionSink)) + response = teeSink->data; auto exc = code == CURLE_ABORTED_BY_CALLBACK && _isInterrupted - ? FileTransferError(Interrupted, fmt("%s of '%s' was interrupted", request.verb(), request.uri)) + ? FileTransferError(Interrupted, response, fmt("%s of '%s' was interrupted", request.verb(), request.uri)) : httpStatus != 0 - ? FileTransferError(err, + ? FileTransferError(err, response, fmt("unable to %s '%s': HTTP error %d", request.verb(), request.uri, httpStatus) + (code == CURLE_OK ? "" : fmt(" (curl error: %s)", curl_easy_strerror(code))) ) - : FileTransferError(err, + : FileTransferError(err, response, fmt("unable to %s '%s': %s (%d)", request.verb(), request.uri, curl_easy_strerror(code), code)); @@ -675,7 +689,7 @@ struct curlFileTransfer : public FileTransfer auto s3Res = s3Helper.getObject(bucketName, key); FileTransferResult res; if (!s3Res.data) - throw FileTransferError(NotFound, fmt("S3 object '%s' does not exist", request.uri)); + throw FileTransferError(NotFound, nullptr, fmt("S3 object '%s' does not exist", request.uri)); res.data = s3Res.data; callback(std::move(res)); #else diff --git a/src/libstore/filetransfer.hh b/src/libstore/filetransfer.hh index 11dca2fe0..8e31a9e42 100644 --- a/src/libstore/filetransfer.hh +++ b/src/libstore/filetransfer.hh @@ -103,9 +103,10 @@ class FileTransferError : public Error { public: FileTransfer::Error error; + std::shared_ptr response; // intentionally optional template - FileTransferError(FileTransfer::Error error, const Args & ... args) - : Error(args...), error(error) + FileTransferError(FileTransfer::Error error, std::shared_ptr response, const Args & ... args) + : Error(args...), error(error), response(response) { } }; From 74b219ef6e5e171c56c8ad7385969e0d0df09ed8 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 18 Jun 2020 14:48:45 +0000 Subject: [PATCH 5/8] Adjust FileTransferError message to use opt response --- src/libstore/filetransfer.cc | 12 ++++++++++++ src/libstore/filetransfer.hh | 7 ++++--- src/libutil/error.hh | 3 +-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index d89f8388f..fa8ad33f5 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -834,6 +834,18 @@ void FileTransfer::download(FileTransferRequest && request, Sink & sink) } } +template +FileTransferError::FileTransferError(FileTransfer::Error error, std::shared_ptr response, const Args & ... args) + : Error(args...), error(error), response(response) +{ + const auto hf = hintfmt(args...); + if (response) { + err.hint = hintfmt("%1%\n\nresponse body:\n\n%2%", normaltxt(hf.str()), response); + } else { + err.hint = hf; + } +} + bool isUri(const string & s) { if (s.compare(0, 8, "channel:") == 0) return true; diff --git a/src/libstore/filetransfer.hh b/src/libstore/filetransfer.hh index 8e31a9e42..25ade0add 100644 --- a/src/libstore/filetransfer.hh +++ b/src/libstore/filetransfer.hh @@ -104,10 +104,11 @@ class FileTransferError : public Error public: FileTransfer::Error error; std::shared_ptr response; // intentionally optional + template - FileTransferError(FileTransfer::Error error, std::shared_ptr response, const Args & ... args) - : Error(args...), error(error), response(response) - { } + FileTransferError(FileTransfer::Error error, std::shared_ptr response, const Args & ... args); + + virtual const char* sname() const override { return "FileTransferError"; } }; bool isUri(const string & s); diff --git a/src/libutil/error.hh b/src/libutil/error.hh index 1e6102ce1..ac9d2e494 100644 --- a/src/libutil/error.hh +++ b/src/libutil/error.hh @@ -173,9 +173,8 @@ public: template SysError(const Args & ... args) - :Error("") + : Error(""), errNo(errno) { - errNo = errno; auto hf = hintfmt(args...); err.hint = hintfmt("%1%: %2%", normaltxt(hf.str()), strerror(errNo)); } From 639e20dc3ed9c5b28138285653912de78fe0507f Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 18 Jun 2020 17:54:16 +0000 Subject: [PATCH 6/8] Prevent '%' in URL from causing crashes We have a larger problem that passsing computed strings to the first variable argument of many exception constructors is unsafe because that first variable argument is interpreted not as a plain string, but format string, and if it contains '%' boost::format will abort, since there are no arguments to the format string. In this particular instance '%' was used as part of an escape code in a URL, which, when the download failed, caused Nix to abort displaying the `FileTransferError`. --- src/libstore/filetransfer.cc | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 531b85af8..e795da860 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -410,16 +410,13 @@ struct curlFileTransfer : public FileTransfer auto exc = code == CURLE_ABORTED_BY_CALLBACK && _isInterrupted - ? FileTransferError(Interrupted, fmt("%s of '%s' was interrupted", request.verb(), request.uri)) + ? FileTransferError(Interrupted, "%s of '%s' was interrupted", request.verb(), request.uri) : httpStatus != 0 - ? FileTransferError(err, - fmt("unable to %s '%s': HTTP error %d", - request.verb(), request.uri, httpStatus) - + (code == CURLE_OK ? "" : fmt(" (curl error: %s)", curl_easy_strerror(code))) - ) - : FileTransferError(err, - fmt("unable to %s '%s': %s (%d)", - request.verb(), request.uri, curl_easy_strerror(code), code)); + ? FileTransferError(err, "unable to %s '%s': HTTP error %d%s", + request.verb(), request.uri, httpStatus, + code == CURLE_OK ? "" : fmt(" (curl error: %s)", curl_easy_strerror(code))) + : FileTransferError(err, "unable to %s '%s': %s (%d)", + request.verb(), request.uri, curl_easy_strerror(code), code); /* If this is a transient error, then maybe retry the download after a while. If we're writing to a @@ -675,7 +672,7 @@ struct curlFileTransfer : public FileTransfer auto s3Res = s3Helper.getObject(bucketName, key); FileTransferResult res; if (!s3Res.data) - throw FileTransferError(NotFound, fmt("S3 object '%s' does not exist", request.uri)); + throw FileTransferError(NotFound, "S3 object '%s' does not exist", request.uri); res.data = s3Res.data; callback(std::move(res)); #else From 1b23fe4afb8ed2c41604a1ed19cf3d49c34f46d1 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 18 Jun 2020 19:03:10 +0000 Subject: [PATCH 7/8] Fix bugs - Bad dynamic cast target ...classic - std::shared_ptr need explicit deref --- src/libstore/filetransfer.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 9ae682bbb..8b66cbdad 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -420,7 +420,7 @@ struct curlFileTransfer : public FileTransfer std::shared_ptr response; if (decompressionSink) - if (auto teeSink = std::dynamic_pointer_cast>>(decompressionSink)) + if (auto teeSink = std::dynamic_pointer_cast>>(decompressionSink)) response = teeSink->data; auto exc = code == CURLE_ABORTED_BY_CALLBACK && _isInterrupted @@ -837,7 +837,7 @@ FileTransferError::FileTransferError(FileTransfer::Error error, std::shared_ptr< { const auto hf = hintfmt(args...); if (response) { - err.hint = hintfmt("%1%\n\nresponse body:\n\n%2%", normaltxt(hf.str()), response); + err.hint = hintfmt("%1%\n\nresponse body:\n\n%2%", normaltxt(hf.str()), *response); } else { err.hint = hf; } From 0ca9744694a5294e995fcddc11f5f195c84036a4 Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Mon, 20 Jul 2020 15:56:52 -0400 Subject: [PATCH 8/8] Use heuristics to decide when to show the response Due to https://github.com/NixOS/nix/issues/3841 we don't know how print different messages for different verbosity levels. --- src/libstore/filetransfer.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 4d35bd2b5..4149f8155 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -845,8 +845,11 @@ FileTransferError::FileTransferError(FileTransfer::Error error, std::shared_ptr< : Error(args...), error(error), response(response) { const auto hf = hintfmt(args...); - if (response) { - err.hint = hintfmt("%1%\n\nresponse body:\n\n%2%", normaltxt(hf.str()), *response); + // FIXME: Due to https://github.com/NixOS/nix/issues/3841 we don't know how + // to print different messages for different verbosity levels. For now + // we add some heuristics for detecting when we want to show the response. + if (response && (response->size() < 1024 || response->find("") != string::npos)) { + err.hint = hintfmt("%1%\n\nresponse body:\n\n%2%", normaltxt(hf.str()), *response); } else { err.hint = hf; }