From de58cd6e80d3ab2ae6ec9e0a5a12c8e0b003f308 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Wed, 22 Jan 2025 20:10:41 +0100 Subject: [PATCH] FileTransfer: fix race condition on awaitData MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There's a race condition where awaitData could early-return for data coming from a 404 response or similar and thus not rethrow the exception that is forthcoming, and a related race during transfer setup (which could retry a transfer *twice* per retry round). This would then cause substitution failures like below since the exception isn't caught in HttpBinaryCacheStore::getFile as intended, but instead by an exception handler downstream of `drain()` which would error out the entire operation. Symptom: ยป nix-build ./docs-service.nix -o "docs-service-result" error: unable to download 'https://cache.nixos.org/7mr3fy8w66gi5inmf0jkkkl90lxy4jyg.narinfo': HTTP e rror 404 () response body: This is kind of a hack in how it is implemented: it assumes that you can't intentionally be receiving a large unsuccessful response since in such a case, `awaitData` will wait for finish() to be called to throw an exception and will never escape until the download finishes, while continuing to buffer the entire response into memory, which could be bad if an error response had a large payload. That said, nobody is sending Lix 1GiB of 404, so meh I guess, and this is how it is seemingly intended to work. That was a design flaw of the thing before any of the Lix team got our paws on it. I tested this by adding _exit(0) inside the expected exception catch and then running the offending command repeatedly to see if the symptom ever appeared again, and it did not. Needs cherry-pick to 2.92 and a 2.92.1 release once reviewed. Fixes: https://git.lix.systems/lix-project/lix/issues/635 Co-Authored-By: lix@jade.fyi Change-Id: If54f6eeaad60b5ca9d5b77d4d9232da1d295e7d1 --- lix/libstore/filetransfer.cc | 37 +++++++++++++++++++---------- tests/unit/libstore/filetransfer.cc | 9 +++++++ 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/lix/libstore/filetransfer.cc b/lix/libstore/filetransfer.cc index 967e40954..d15550b0b 100644 --- a/lix/libstore/filetransfer.cc +++ b/lix/libstore/filetransfer.cc @@ -56,7 +56,7 @@ struct curlFileTransfer : public FileTransfer std::unique_ptr uploadData; Sync downloadState; std::condition_variable downloadEvent; - bool headersDone = false; + bool headersDone = false, metadataReturned = false; std::promise metadataPromise; std::string statusMsg; @@ -193,7 +193,7 @@ struct curlFileTransfer : public FileTransfer { auto state = downloadState.lock(); assert(!state->done && !state->exc); - if (!headersDone) { + if (!metadataReturned) { metadataPromise.set_exception(ex); } state->exc = ex; @@ -212,15 +212,20 @@ struct curlFileTransfer : public FileTransfer return; } + auto status = getHTTPStatus(); + char * effectiveUriCStr = nullptr; curl_easy_getinfo(req.get(), CURLINFO_EFFECTIVE_URL, &effectiveUriCStr); if (effectiveUriCStr) { result.effectiveUri = effectiveUriCStr; } - result.cached = getHTTPStatus() == 304; + result.cached = status == 304; + if (successfulStatuses.contains(status)) { + metadataPromise.set_value(result); + metadataReturned = true; + } - metadataPromise.set_value(result); headersDone = true; } @@ -786,7 +791,8 @@ struct curlFileTransfer : public FileTransfer , data(std::move(data)) , noBody(noBody) { - metadata = withRetries([&] { return startTransfer(uri); }); + auto setup = [&] { return startTransfer(uri); }; + metadata = withRetries(setup, setup); } ~TransferSource() @@ -801,15 +807,17 @@ struct curlFileTransfer : public FileTransfer } } - auto withRetries(auto fn) -> decltype(fn()) + auto withRetries(auto && initial, auto && retry) -> decltype(initial()) { std::optional retryContext; while (true) { try { if (retryContext) { - attemptRetry(*retryContext); + prepareRetry(*retryContext); + return retry(); + } else { + return initial(); } - return fn(); } catch (FileTransferError & e) { // If this is a transient error, then maybe retry after a while. after any // bytes have been received we require range support to proceed, otherwise @@ -843,7 +851,7 @@ struct curlFileTransfer : public FileTransfer } } - bool attemptRetry(const std::string & context) + void prepareRetry(const std::string & context) { thread_local std::minstd_rand random{std::random_device{}()}; std::uniform_real_distribution<> dist(0.0, 0.5); @@ -855,7 +863,10 @@ struct curlFileTransfer : public FileTransfer } std::this_thread::sleep_for(std::chrono::milliseconds(ms)); + } + void restartTransfer() + { // use the effective URI of the previous transfer for retries. this avoids // some silent corruption if a redirect changes between starting and retry const auto & uri = metadata.effectiveUri.empty() ? this->uri : metadata.effectiveUri; @@ -868,13 +879,11 @@ struct curlFileTransfer : public FileTransfer metadata.immutableUrl.value_or(""), newMeta.immutableUrl.value_or("") ); - - return true; } bool awaitData() { - return withRetries([&] { + auto waitForData = [&] { /* Grab data if available, otherwise wait for the download thread to wake us up. */ while (buffered.empty()) { @@ -896,6 +905,10 @@ struct curlFileTransfer : public FileTransfer } return true; + }; + return withRetries(waitForData, [&] { + restartTransfer(); + return waitForData(); }); } diff --git a/tests/unit/libstore/filetransfer.cc b/tests/unit/libstore/filetransfer.cc index 52369ee11..7e14430a4 100644 --- a/tests/unit/libstore/filetransfer.cc +++ b/tests/unit/libstore/filetransfer.cc @@ -426,4 +426,13 @@ TEST(FileTransfer, DISABLED_interrupt) ASSERT_THROW(ft->download(fmt("http://[::1]:%d/index", port)).second->drain(), FileTransferError); } +TEST(FileTransfer, setupErrorsAreMetadata) +{ + auto [port, srv] = serveHTTP({ + {"404 try again later", "content-length: 1\r\n", [] { return "X"; }}, + }); + auto ft = makeFileTransfer(0); + ASSERT_THROW(ft->upload(fmt("http://[::1]:%d", port), ""), FileTransferError); +} + }