From e5c4de34c5cd536231dc07f8fdf936bc0bf5eeef Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Mon, 16 Jun 2025 18:51:59 +0200 Subject: [PATCH] libstore: eagerly mark daemon connections as bad on local errors do not rely on Source/Sink `good()` or delayed guessing about whether an exception was thrown by the daemon or not. mark connections as bad for all local errors happening while communication is ongoing instead, and leave it valid only when an exception was provided by the remote. we may drop connections a bit too eagerly now, but all cases in which that happens were vulnerable to protocol desynchronization. there are still a few windows for this to happen left, but those are unfixable. Change-Id: Iefaa66c552092c436b9de77aa3f8e09f847a966e --- lix/libstore/remote-store-connection.hh | 44 +++++++++++++++---------- lix/libstore/remote-store.cc | 28 ++++++++-------- lix/libutil/serialise.hh | 14 +++++++- 3 files changed, 53 insertions(+), 33 deletions(-) diff --git a/lix/libstore/remote-store-connection.hh b/lix/libstore/remote-store-connection.hh index 9d16fe9e0..e2bdeca51 100644 --- a/lix/libstore/remote-store-connection.hh +++ b/lix/libstore/remote-store-connection.hh @@ -120,7 +120,6 @@ struct RemoteStore::ConnectionHandle { Pool::Handle handle; Sync & handlerThreads; - bool daemonException = false; ConnectionHandle( Pool::Handle && handle, Sync & handlerThreads @@ -133,13 +132,9 @@ struct RemoteStore::ConnectionHandle ConnectionHandle(ConnectionHandle && h) : handle(std::move(h.handle)) , handlerThreads(h.handlerThreads) - , daemonException(h.daemonException) { - h.daemonException = false; } - ~ConnectionHandle(); - RemoteStore::Connection & operator * () { return *handle; } RemoteStore::Connection * operator -> () { return &*handle; } @@ -161,23 +156,33 @@ struct RemoteStore::ConnectionHandle // and serialize all *preceding* arguments normally before handing over to // the subframing layer (which is then responsible for any error handling) if constexpr (requires { *handle->to << std::declval(); }) { - StringSink msg; - ((msg << std::forward(args)), ...); - StringSource{msg.s}.drainInto(*handle->to); - handle->to->flush(); + try { + StringSink msg; + ((msg << std::forward(args)), ...); + StringSource{msg.s}.drainInto(*handle->to); + handle->to->flush(); + } catch (...) { + handle.markBad(); + throw; + } LIX_TRY_AWAIT(processStderr()); } else { using ImmediateArgsIdxs = std::make_index_sequence; AllArgsT allArgs(std::forward(args)...); [&](std::integer_sequence) { - StringSink msg; - ((msg << std::forward>( - std::get(allArgs) - )), - ...); - StringSource{msg.s}.drainInto(*handle->to); - handle->to->flush(); + try { + StringSink msg; + ((msg << std::forward>( + std::get(allArgs) + )), + ...); + StringSource{msg.s}.drainInto(*handle->to); + handle->to->flush(); + } catch (...) { + handle.markBad(); + throw; + } }(ImmediateArgsIdxs{}); LIX_TRY_AWAIT(withFramedSinkAsync(std::get(allArgs))); @@ -186,7 +191,12 @@ struct RemoteStore::ConnectionHandle if constexpr (std::is_void_v) { co_return result::success(); } else { - co_return WorkerProto::Serialise::read(*handle); + try { + co_return WorkerProto::Serialise::read(*handle); + } catch (...) { + handle.markBad(); + throw; + } } } catch (...) { co_return result::current_exception(); diff --git a/lix/libstore/remote-store.cc b/lix/libstore/remote-store.cc index 9f634b0f8..eb8031e91 100644 --- a/lix/libstore/remote-store.cc +++ b/lix/libstore/remote-store.cc @@ -38,8 +38,7 @@ RemoteStore::RemoteStore(const RemoteStoreConfig & config) std::max(1, (int) config.maxConnections), [this]() { return openAndInitConnection(); }, [this](const ref & r) { - return r->to->good() && r->from->good() - && std::chrono::duration_cast( + return std::chrono::duration_cast( std::chrono::steady_clock::now() - r->startTime ) .count() @@ -169,23 +168,15 @@ try { co_return result::current_exception(); } -RemoteStore::ConnectionHandle::~ConnectionHandle() -{ - if (!daemonException && std::uncaught_exceptions()) { - handle.markBad(); - debug("closing daemon connection because of an exception"); - } -} - kj::Promise> RemoteStore::ConnectionHandle::processStderr() try { auto ex = TRY_AWAIT(handle->processStderr()); if (ex.e) { - daemonException = true; - std::rethrow_exception(ex.e); + co_return result::failure(ex.e); } co_return result::success(); } catch (...) { + handle.markBad(); co_return result::current_exception(); } @@ -727,7 +718,12 @@ try { auto conn(TRY_AWAIT(getConnection())); TRY_AWAIT(conn.sendCommand(WorkerProto::Op::NarFromPath, printStorePath(path))); co_return make_box_ptr([](auto conn) -> WireFormatGenerator { - co_yield copyNAR(*conn->from); + try { + co_yield copyNAR(*conn->from); + } catch (...) { + conn.handle.markBad(); + throw; + } }(std::move(conn))); } catch (...) { co_return result::current_exception(); @@ -828,7 +824,7 @@ RemoteStore::ConnectionHandle::FramedSinkHandler::~FramedSinkHandler() noexcept( // handle multiple exceptions anyway the safest path is to simply drop // the remote (possibly Interrupted) exception when called for unwind. if (ex && std::uncaught_exceptions() == 0) { - std::rethrow_exception(ex); + throw FramedSink::RemoteError(ex); } } @@ -843,8 +839,10 @@ try { sink.flush(); } co_return result::success(); +} catch (FramedSink::RemoteError & e) { + co_return result::failure(e.e); } catch (...) { + handle.markBad(); co_return result::current_exception(); } - } diff --git a/lix/libutil/serialise.hh b/lix/libutil/serialise.hh index b7d4ffa40..fc849c90f 100644 --- a/lix/libutil/serialise.hh +++ b/lix/libutil/serialise.hh @@ -1,8 +1,10 @@ #pragma once ///@file +#include #include +#include "error.hh" #include "lix/libutil/charptr-cast.hh" #include "lix/libutil/generator.hh" #include "lix/libutil/io-buffer.hh" @@ -559,6 +561,16 @@ struct FramedSink : nix::BufferedSink BufferedSink & to; std::exception_ptr & ex; + struct RemoteError : BaseException + { + std::exception_ptr e; + + RemoteError(std::exception_ptr e) + : e(e) // NOLINT(bugprone-throw-keyword-missing): intentional copy + { + } + }; + FramedSink(BufferedSink & to, std::exception_ptr & ex) : to(to), ex(ex) { } @@ -579,7 +591,7 @@ struct FramedSink : nix::BufferedSink if (ex) { auto ex2 = ex; ex = nullptr; - std::rethrow_exception(ex2); + throw RemoteError{ex2}; } to << data.size(); to(data);