From 350bf7e42db2e81795208766001d982877bf0279 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Wed, 9 Apr 2025 11:28:50 +0000 Subject: [PATCH] Revert "libstore: don't use curl decompression support" This reverts commit bba678e5c51e02fda134369ee08bf52489138fe3. Reason for revert: didn't fix the bug and created new ones (fj#794) Change-Id: I0450205d3041b6c876737151a4051081c1366f1d --- .../rl-next/curl-http2-decompression-fix.md | 14 ---- lix/libstore/filetransfer.cc | 75 +++++-------------- tests/unit/libstore/filetransfer.cc | 32 +------- 3 files changed, 20 insertions(+), 101 deletions(-) delete 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 deleted file mode 100644 index aea393c93..000000000 --- a/doc/manual/rl-next/curl-http2-decompression-fix.md +++ /dev/null @@ -1,14 +0,0 @@ ---- -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 01d0a0580..f8c97086a 100644 --- a/lix/libstore/filetransfer.cc +++ b/lix/libstore/filetransfer.cc @@ -1,6 +1,4 @@ #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" @@ -37,14 +35,6 @@ 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; @@ -61,13 +51,13 @@ struct curlFileTransfer : public FileTransfer }; std::string uri; - FileTransferResultWithEncoding result; + FileTransferResult 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; @@ -94,17 +84,6 @@ 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, @@ -121,8 +100,17 @@ struct curlFileTransfer : public FileTransfer if (req == nullptr) { throw FileTransferError(Misc, {}, "could not allocate curl handle"); } - for (auto it = headers.begin(); it != headers.end(); ++it) { - appendCurlHeader(it->first, it->second); + 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"); + } } if (verbosity >= lvlVomit) { @@ -132,14 +120,7 @@ struct curlFileTransfer : public FileTransfer curl_easy_setopt(req.get(), CURLOPT_URL, uri.c_str()); curl_easy_setopt(req.get(), CURLOPT_FOLLOWLOCATION, 1L); - { - // 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_ACCEPT_ENCODING, ""); // all of them! curl_easy_setopt(req.get(), CURLOPT_MAXREDIRS, 10); curl_easy_setopt(req.get(), CURLOPT_NOSIGNAL, 1); curl_easy_setopt(req.get(), CURLOPT_USERAGENT, @@ -289,7 +270,6 @@ 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) { @@ -310,10 +290,6 @@ 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; @@ -789,7 +765,7 @@ struct curlFileTransfer : public FileTransfer auto source = make_box_ptr(*this, uri, headers, std::move(data), noBody); source->awaitData(); - return {source->metadata, make_box_ptr(std::move(source))}; + return {source->metadata, std::move(source)}; } struct TransferSource : Source @@ -802,7 +778,7 @@ struct curlFileTransfer : public FileTransfer ActivityId parentAct = getCurActivity(); std::shared_ptr transfer; - FileTransferResultWithEncoding metadata; + FileTransferResult metadata; std::string chunk; std::string_view buffered; @@ -864,7 +840,7 @@ struct curlFileTransfer : public FileTransfer } } - FileTransferResultWithEncoding startTransfer(const std::string & uri, curl_off_t offset = 0) + FileTransferResult startTransfer(const std::string & uri, curl_off_t offset = 0) { attempt += 1; auto uploadData = data ? std::optional(std::string_view(*data)) : std::nullopt; @@ -911,7 +887,6 @@ struct curlFileTransfer : public FileTransfer metadata.immutableUrl.value_or(""), newMeta.immutableUrl.value_or("") ); - throwChangedTarget("compression", metadata.encoding, newMeta.encoding); } bool awaitData() @@ -963,22 +938,6 @@ 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 a44a7b892..7e14430a4 100644 --- a/tests/unit/libstore/filetransfer.cc +++ b/tests/unit/libstore/filetransfer.cc @@ -25,7 +25,6 @@ #endif using namespace std::chrono_literals; -using namespace std::string_literals; namespace { @@ -258,16 +257,12 @@ TEST(FileTransfer, NOT_ON_DARWIN(defersFailures)) ASSERT_THROW(src->drain(), FileTransferError); } -class FileTransferEncoding : public testing::TestWithParam> -{}; - -TEST_P(FileTransferEncoding, NOT_ON_DARWIN(handlesContentEncoding)) +TEST(FileTransfer, NOT_ON_DARWIN(handlesContentEncoding)) { std::string original = "Test data string"; - auto [method, compressed] = GetParam(); + std::string compressed = compress("gzip", original); - auto [port, srv] = - serveHTTP("200 ok", "content-encoding: " + method + "\r\n", [&] { return compressed; }); + auto [port, srv] = serveHTTP("200 ok", "content-encoding: gzip\r\n", [&] { return compressed; }); auto ft = makeFileTransfer(); StringSink sink; @@ -275,27 +270,6 @@ TEST_P(FileTransferEncoding, 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({