From bba678e5c51e02fda134369ee08bf52489138fe3 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sat, 8 Mar 2025 16:27:49 +0100 Subject: [PATCH] libstore: don't use curl decompression support it's broken with http2 and transfer flow control. cf fj#662 Change-Id: Iaf6312bfcefa18d168faef47f57481199dd30b8d --- .../rl-next/curl-http2-decompression-fix.md | 14 ++++ lix/libstore/filetransfer.cc | 75 ++++++++++++++----- tests/unit/libstore/filetransfer.cc | 32 +++++++- 3 files changed, 101 insertions(+), 20 deletions(-) create mode 100644 doc/manual/rl-next/curl-http2-decompression-fix.md diff --git a/doc/manual/rl-next/curl-http2-decompression-fix.md b/doc/manual/rl-next/curl-http2-decompression-fix.md new file mode 100644 index 000000000..aea393c93 --- /dev/null +++ b/doc/manual/rl-next/curl-http2-decompression-fix.md @@ -0,0 +1,14 @@ +--- +synopsis: "Fix curl error `A value or data field grew larger than allowed`" +cls: [2780] +category: Fixes +credits: horrors +--- + +2.92 started using curl-provided HTTP decompression code, but it as discovered +that curl has [a bug] that effectively breaks its decompression code on HTTP/2 +transfers. We have partially rolled back our changes and no longer use builtin +decompression methods provided by curl, but have kept the refusal of bzip2 and +xz content encodings introduced with 2.92 since they are not in the HTTP spec. + +[a bug]: https://git.lix.systems/lix-project/lix/issues/662 diff --git a/lix/libstore/filetransfer.cc b/lix/libstore/filetransfer.cc index f8c97086a..01d0a0580 100644 --- a/lix/libstore/filetransfer.cc +++ b/lix/libstore/filetransfer.cc @@ -1,4 +1,6 @@ #include "lix/libstore/filetransfer.hh" +#include "lix/libutil/box_ptr.hh" +#include "lix/libutil/compression.hh" #include "lix/libutil/namespaces.hh" #include "lix/libstore/globals.hh" #include "lix/libstore/store-api.hh" @@ -35,6 +37,14 @@ FileTransferSettings fileTransferSettings; static GlobalConfig::Register rFileTransferSettings(&fileTransferSettings); +namespace { +struct FileTransferResultWithEncoding : FileTransferResult +{ + // empty string means identity (cf makeDecompressionSource) + std::string encoding; +}; +} + struct curlFileTransfer : public FileTransfer { std::unique_ptr curlm; @@ -51,13 +61,13 @@ struct curlFileTransfer : public FileTransfer }; std::string uri; - FileTransferResult result; + FileTransferResultWithEncoding result; Activity act; std::unique_ptr uploadData; Sync downloadState; std::condition_variable downloadEvent; bool headersDone = false, metadataReturned = false; - std::promise metadataPromise; + std::promise metadataPromise; std::string statusMsg; uint64_t bodySize = 0; @@ -84,6 +94,17 @@ struct curlFileTransfer : public FileTransfer return uploadData ? "upload" : "download"; } + void appendCurlHeader(std::string_view name, std::string_view value) + { + auto header = fmt("%s: %s", name, value); + if (auto next = curl_slist_append(requestHeaders.get(), header.c_str())) { + (void) requestHeaders.release(); // next now owns this pointer + requestHeaders.reset(next); + } else { + throw FileTransferError(Misc, {}, "could not allocate curl request headers"); + } + } + TransferItem(const std::string & uri, const Headers & headers, ActivityId parentAct, @@ -100,17 +121,8 @@ struct curlFileTransfer : public FileTransfer if (req == nullptr) { throw FileTransferError(Misc, {}, "could not allocate curl handle"); } - for (auto it = headers.begin(); it != headers.end(); ++it){ - if (auto next = curl_slist_append( - requestHeaders.get(), fmt("%s: %s", it->first, it->second).c_str() - ); - next != nullptr) - { - (void) requestHeaders.release(); // next now owns this pointer - requestHeaders.reset(next); - } else { - throw FileTransferError(Misc, {}, "could not allocate curl request headers"); - } + for (auto it = headers.begin(); it != headers.end(); ++it) { + appendCurlHeader(it->first, it->second); } if (verbosity >= lvlVomit) { @@ -120,7 +132,14 @@ struct curlFileTransfer : public FileTransfer curl_easy_setopt(req.get(), CURLOPT_URL, uri.c_str()); curl_easy_setopt(req.get(), CURLOPT_FOLLOWLOCATION, 1L); - curl_easy_setopt(req.get(), CURLOPT_ACCEPT_ENCODING, ""); // all of them! + { + // curl builtin decompression disabled due to bugs, instead we add + // an accept-encoding header of our own and decompress manually :( + // we don't support deflate because libarchive also doesn't either + // cf https://git.lix.systems/lix-project/lix/issues/662 for infos + // curl_easy_setopt(req.get(), CURLOPT_ACCEPT_ENCODING, ""); // all of them! + appendCurlHeader("Accept-Encoding", "gzip, br, zstd"); + } curl_easy_setopt(req.get(), CURLOPT_MAXREDIRS, 10); curl_easy_setopt(req.get(), CURLOPT_NOSIGNAL, 1); curl_easy_setopt(req.get(), CURLOPT_USERAGENT, @@ -270,6 +289,7 @@ struct curlFileTransfer : public FileTransfer static std::regex statusLine("HTTP/[^ ]+ +[0-9]+(.*)", std::regex::extended | std::regex::icase); if (std::smatch match; std::regex_match(line, match, statusLine)) { statusMsg = trim(match.str(1)); + result.encoding = ""; } else { auto i = line.find(':'); if (i != std::string::npos) { @@ -290,6 +310,10 @@ struct curlFileTransfer : public FileTransfer } else debug("got invalid link header '%s'", value); } + + else if (name == "content-encoding") { + result.encoding = trim(line.substr(i + 1)); + } } } return realSize; @@ -765,7 +789,7 @@ struct curlFileTransfer : public FileTransfer auto source = make_box_ptr(*this, uri, headers, std::move(data), noBody); source->awaitData(); - return {source->metadata, std::move(source)}; + return {source->metadata, make_box_ptr(std::move(source))}; } struct TransferSource : Source @@ -778,7 +802,7 @@ struct curlFileTransfer : public FileTransfer ActivityId parentAct = getCurActivity(); std::shared_ptr transfer; - FileTransferResult metadata; + FileTransferResultWithEncoding metadata; std::string chunk; std::string_view buffered; @@ -840,7 +864,7 @@ struct curlFileTransfer : public FileTransfer } } - FileTransferResult startTransfer(const std::string & uri, curl_off_t offset = 0) + FileTransferResultWithEncoding startTransfer(const std::string & uri, curl_off_t offset = 0) { attempt += 1; auto uploadData = data ? std::optional(std::string_view(*data)) : std::nullopt; @@ -887,6 +911,7 @@ struct curlFileTransfer : public FileTransfer metadata.immutableUrl.value_or(""), newMeta.immutableUrl.value_or("") ); + throwChangedTarget("compression", metadata.encoding, newMeta.encoding); } bool awaitData() @@ -938,6 +963,22 @@ struct curlFileTransfer : public FileTransfer } }; + struct DecompressionWrapper : Source + { + box_ptr wrapped; + std::unique_ptr decompressor; + + explicit DecompressionWrapper(box_ptr inner) : wrapped(std::move(inner)) {} + + size_t read(char * data, size_t len) override + { + if (!decompressor) { + decompressor = makeDecompressionSource(wrapped->metadata.encoding, *wrapped); + } + return decompressor->read(data, len); + } + }; + bool exists(const std::string & uri, const Headers & headers) override { try { diff --git a/tests/unit/libstore/filetransfer.cc b/tests/unit/libstore/filetransfer.cc index 7e14430a4..a44a7b892 100644 --- a/tests/unit/libstore/filetransfer.cc +++ b/tests/unit/libstore/filetransfer.cc @@ -25,6 +25,7 @@ #endif using namespace std::chrono_literals; +using namespace std::string_literals; namespace { @@ -257,12 +258,16 @@ TEST(FileTransfer, NOT_ON_DARWIN(defersFailures)) ASSERT_THROW(src->drain(), FileTransferError); } -TEST(FileTransfer, NOT_ON_DARWIN(handlesContentEncoding)) +class FileTransferEncoding : public testing::TestWithParam> +{}; + +TEST_P(FileTransferEncoding, NOT_ON_DARWIN(handlesContentEncoding)) { std::string original = "Test data string"; - std::string compressed = compress("gzip", original); + auto [method, compressed] = GetParam(); - auto [port, srv] = serveHTTP("200 ok", "content-encoding: gzip\r\n", [&] { return compressed; }); + auto [port, srv] = + serveHTTP("200 ok", "content-encoding: " + method + "\r\n", [&] { return compressed; }); auto ft = makeFileTransfer(); StringSink sink; @@ -270,6 +275,27 @@ TEST(FileTransfer, NOT_ON_DARWIN(handlesContentEncoding)) EXPECT_EQ(sink.s, original); } +INSTANTIATE_TEST_SUITE_P( + , + FileTransferEncoding, + testing::Values( + std::pair{ + "gzip", + "\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\x03\x0b\x49\x2d\x2e\x51\x48\x49\x2c\x49\x54\x28" + "\x2e\x29\xca\xcc\x4b\x07\x00\x34\xfd\xff\xfa\x10\x00\x00\x00"s + }, + std::pair{ + "zstd", + "\x28\xb5\x2f\xfd\x04\x58\x81\x00\x00\x54\x65\x73\x74\x20\x64\x61\x74\x61\x20\x73\x74" + "\x72\x69\x6e\x67\x5e\xc9\x0e\xca"s + }, + std::pair{ + "br", + "\x8f\x07\x80\x54\x65\x73\x74\x20\x64\x61\x74\x61\x20\x73\x74\x72\x69\x6e\x67\x03"s + } + ) +); + TEST(FileTransfer, usesIntermediateLinkHeaders) { auto [port, srv] = serveHTTP({