From 6c777476c9e97abfc5232f0707985caf6df2baea Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Thu, 4 Apr 2024 17:31:01 +0200 Subject: [PATCH 1/7] libutil: guard Finally against invalid exception throws throwing exceptions is fine, but throwing exceptions during exception handling is hard enough to do correctly that we should just forbid it entirely out of an overabundance of caution. in cases where terminate is the correct answer the users of Finally must call it manually now. Change-Id: Ia51a2cb4a0638500550bfabc89cf01a6d8098983 --- src/libutil/finally.hh | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/libutil/finally.hh b/src/libutil/finally.hh index f2293e5d4..cbfd6195b 100644 --- a/src/libutil/finally.hh +++ b/src/libutil/finally.hh @@ -1,6 +1,9 @@ #pragma once ///@file +#include +#include + /** * A trivial class to run a function at the end of a scope. */ @@ -19,5 +22,25 @@ public: Finally(Finally &&other) : fun(std::move(other.fun)) { other.movedFrom = true; } - ~Finally() noexcept(false) { if (!movedFrom) fun(); } + ~Finally() noexcept(false) + { + try { + if (!movedFrom) + fun(); + } catch (...) { + // finally may only throw an exception if exception handling is not already + // in progress. if handling *is* in progress we have to return cleanly here + // but are still prohibited from doing so since eating the exception would, + // in almost all cases, mess up error handling even more. the only good way + // to handle this is to abort entirely and leave a message, so we'll assert + // (and rethrow anyway, just as a defense against possible NASSERT builds.) + if (std::uncaught_exceptions()) { + assert(false && + "Finally function threw an exception during exception handling. " + "this is not what you want, please use some other methods (like " + "std::promise or async) instead."); + } + throw; + } + } }; From 821ad98beb1a915ea7a456c274bcfca9e059ba91 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 5 Apr 2024 03:51:45 +0200 Subject: [PATCH 2/7] Revert "libutil: drop Fs{Source,Sink}::good" This reverts commit 1340807e30dba4b3972c31f02861bbaeaeb60e61. Change-Id: I34d2a80eb3c3e9d79cb02b92cd1189da32d18cb6 --- src/libstore/remote-store.cc | 8 +++----- src/libutil/serialise.cc | 23 ++++++++++++++++++++--- src/libutil/serialise.hh | 11 +++++++++++ 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 2ce047acd..2373bbdc7 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -39,7 +39,9 @@ RemoteStore::RemoteStore(const Params & params) }, [this](const ref & r) { return - std::chrono::duration_cast( + r->to.good() + && r->from.good() + && std::chrono::duration_cast( std::chrono::steady_clock::now() - r->startTime).count() < maxConnectionAge; } )) @@ -178,10 +180,6 @@ void RemoteStore::ConnectionHandle::processStderr(Sink * sink, Source * source, m.find("Derive([") != std::string::npos) throw Error("%s, this might be because the daemon is too old to understand dependencies on dynamic derivations. Check to see if the raw derivation is in the form '%s'", std::move(m), "DrvWithVersion(..)"); } - // the daemon can still handle more requests, so the connection itself - // is still valid. the current *handle* however should be considered a - // lost cause and abandoned entirely. - handle.release(); throw; } } diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index 6450a9651..692144b75 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -52,7 +52,18 @@ FdSink::~FdSink() void FdSink::writeUnbuffered(std::string_view data) { written += data.size(); - writeFull(fd, data); + try { + writeFull(fd, data); + } catch (SysError & e) { + _good = false; + throw; + } +} + + +bool FdSink::good() +{ + return _good; } @@ -117,13 +128,19 @@ size_t FdSource::readUnbuffered(char * data, size_t len) checkInterrupt(); n = ::read(fd, data, len); } while (n == -1 && errno == EINTR); - if (n == -1) { throw SysError("reading from file"); } - if (n == 0) { throw EndOfFile(std::string(*endOfFileError)); } + if (n == -1) { _good = false; throw SysError("reading from file"); } + if (n == 0) { _good = false; throw EndOfFile(std::string(*endOfFileError)); } read += n; return n; } +bool FdSource::good() +{ + return _good; +} + + size_t StringSource::read(char * data, size_t len) { if (pos == s.size()) throw EndOfFile("end of string reached"); diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index e6290a652..d1c791823 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -18,6 +18,7 @@ struct Sink { virtual ~Sink() { } virtual void operator () (std::string_view data) = 0; + virtual bool good() { return true; } }; /** @@ -79,6 +80,8 @@ struct Source */ virtual size_t read(char * data, size_t len) = 0; + virtual bool good() { return true; } + void drainInto(Sink & sink); std::string drain(); @@ -133,6 +136,11 @@ struct FdSink : BufferedSink ~FdSink(); void writeUnbuffered(std::string_view data) override; + + bool good() override; + +private: + bool _good = true; }; @@ -157,8 +165,11 @@ struct FdSource : BufferedSource return *this; } + bool good() override; protected: size_t readUnbuffered(char * data, size_t len) override; +private: + bool _good = true; }; From c77b6e1fdd2ecacaa044b84dc89e89565337ddaa Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 5 Apr 2024 03:51:52 +0200 Subject: [PATCH 3/7] Revert "libutil: allow graceful dropping of Pool::Handle" This reverts commit 8075541d82d05347321d35b9934ccee5f82142f4. Change-Id: I05fa6a9de1308a4827a6557cf2807eb47ca64da6 --- src/libutil/pool.hh | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/src/libutil/pool.hh b/src/libutil/pool.hh index bc71083e9..2f6d30130 100644 --- a/src/libutil/pool.hh +++ b/src/libutil/pool.hh @@ -108,19 +108,6 @@ public: Handle(Pool & pool, std::shared_ptr r) : pool(pool), r(r) { } - void drop(bool stillValid) - { - { - auto state_(pool.state.lock()); - if (stillValid) - state_->idle.emplace_back(std::move(r)); - assert(state_->inUse); - state_->inUse--; - } - pool.wakeup.notify_one(); - r = nullptr; - } - public: Handle(Handle && h) : pool(h.pool), r(h.r) { h.r.reset(); } @@ -128,13 +115,15 @@ public: ~Handle() { - if (r) - drop(std::uncaught_exceptions() == 0); - } - - void release() - { - drop(true); + if (!r) return; + { + auto state_(pool.state.lock()); + if (!std::uncaught_exceptions()) + state_->idle.push_back(ref(r)); + assert(state_->inUse); + state_->inUse--; + } + pool.wakeup.notify_one(); } R * operator -> () { return &*r; } From ad3097286782e9773416a929badf54777d9861a1 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Thu, 4 Apr 2024 17:27:27 +0200 Subject: [PATCH 4/7] Revert "libstore: using throwing finally in withFramedSink" This reverts commit 491caad6f62c21ffbcdebe662e63ec0f72e6f3a2. this is not actually legal for nix! throwing exceptions in destructors is fine, but the way nix is set up we'll end up throwing the exception we received from the remote *twice* in some cases, and such cases will cause an immediate terminate without active exception. Change-Id: I74c46b9f26fd791086e4193ec60eb1deb9a5bb2a --- src/libstore/remote-store.cc | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 2373bbdc7..3188d9330 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -1067,15 +1067,27 @@ void RemoteStore::ConnectionHandle::withFramedSink(std::functionto, ex); - fun(sink); - sink.flush(); + { + FramedSink sink((*this)->to, ex); + fun(sink); + sink.flush(); + } + + stderrThread.join(); + if (ex) + std::rethrow_exception(ex); } } From 0b8a17cab6d47c0abf1f604b6dbc6dc92553ed15 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 5 Apr 2024 03:52:23 +0200 Subject: [PATCH 5/7] Revert "libstore: remove one Resource::good flag" This reverts commit 87249eb579bf57f4f09e9fca100588a4d6b90b4c. Change-Id: Ide4c6e00c4155216a17e46671ff47151d7bb85b4 --- src/libstore/legacy-ssh-store.cc | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index e06d4e08d..2d8667a85 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -46,6 +46,7 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor FdSink to; FdSource from; ServeProto::Version remoteVersion; + bool good = true; /** * Coercion to `ServeProto::ReadConn`. This makes it easy to use the @@ -96,7 +97,8 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor , host(host) , connections(make_ref>( std::max(1, (int) maxConnections), - [this]() { return openConnection(); } + [this]() { return openConnection(); }, + [](const ref & r) { return r->good; } )) , master( host, @@ -194,7 +196,12 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor << info.ultimate << info.sigs << renderContentAddress(info.ca); - copyNAR(source, conn->to); + try { + copyNAR(source, conn->to); + } catch (...) { + conn->good = false; + throw; + } conn->to.flush(); } else { @@ -202,7 +209,12 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor conn->to << ServeProto::Command::ImportPaths << 1; - copyNAR(source, conn->to); + try { + copyNAR(source, conn->to); + } catch (...) { + conn->good = false; + throw; + } conn->to << exportMagic << printStorePath(info.path); From 52f741c23af84ebb67f7295600069a9c6e83aec4 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 5 Apr 2024 03:52:34 +0200 Subject: [PATCH 6/7] Revert "libutil: remove Pool::Handle::bad" This reverts commit 792844fb861ea7367ac2316c78fec055363f2f9e. Change-Id: I3ca208b62edfd5cd1199478f75cd2edf19a364f6 --- src/libstore/remote-store.cc | 1 + src/libutil/pool.hh | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 3188d9330..20c1c50f2 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -155,6 +155,7 @@ void RemoteStore::setOptions(Connection & conn) RemoteStore::ConnectionHandle::~ConnectionHandle() { if (!daemonException && std::uncaught_exceptions()) { + handle.markBad(); debug("closing daemon connection because of an exception"); } } diff --git a/src/libutil/pool.hh b/src/libutil/pool.hh index 2f6d30130..b7a749e3a 100644 --- a/src/libutil/pool.hh +++ b/src/libutil/pool.hh @@ -103,6 +103,7 @@ public: private: Pool & pool; std::shared_ptr r; + bool bad = false; friend Pool; @@ -118,7 +119,7 @@ public: if (!r) return; { auto state_(pool.state.lock()); - if (!std::uncaught_exceptions()) + if (!bad && !std::uncaught_exceptions()) state_->idle.push_back(ref(r)); assert(state_->inUse); state_->inUse--; @@ -128,6 +129,8 @@ public: R * operator -> () { return &*r; } R & operator * () { return *r; } + + void markBad() { bad = true; } }; Handle get() From 38dc6f5b69da81dfd21780ef16efaa297f9c2231 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 5 Apr 2024 03:52:51 +0200 Subject: [PATCH 7/7] Revert "libutil: drop Pool resources on exceptional free" This reverts commit de2884b82b376d10de5c400d8e73bc7d98f195d2. Change-Id: I1fa301149d7c2ed3d266a40c15b2d010e12e44e6 --- src/libutil/pool.hh | 9 +-------- tests/unit/libutil/pool.cc | 15 --------------- 2 files changed, 1 insertion(+), 23 deletions(-) diff --git a/src/libutil/pool.hh b/src/libutil/pool.hh index b7a749e3a..1cece71ec 100644 --- a/src/libutil/pool.hh +++ b/src/libutil/pool.hh @@ -1,7 +1,6 @@ #pragma once ///@file -#include #include #include #include @@ -119,7 +118,7 @@ public: if (!r) return; { auto state_(pool.state.lock()); - if (!bad && !std::uncaught_exceptions()) + if (!bad) state_->idle.push_back(ref(r)); assert(state_->inUse); state_->inUse--; @@ -135,12 +134,6 @@ public: Handle get() { - // we do not want to handle the complexity that comes with allocating - // resources during stack unwinding. it would be possible to do this, - // but doing so requires more per-handle bookkeeping to properly free - // resources allocated during unwinding. that effort is not worth it. - assert(std::uncaught_exceptions() == 0); - { auto state_(state.lock()); diff --git a/tests/unit/libutil/pool.cc b/tests/unit/libutil/pool.cc index 3ad4ed3aa..90ee509ba 100644 --- a/tests/unit/libutil/pool.cc +++ b/tests/unit/libutil/pool.cc @@ -109,19 +109,4 @@ namespace nix { ASSERT_NE(h->num, counter); } } - - TEST(Pool, throwingOperationDropsResource) - { - auto createResource = []() { return make_ref(); }; - - Pool pool = Pool((size_t)1, createResource); - - ASSERT_THROW({ - auto _r = pool.get(); - ASSERT_EQ(pool.count(), 1); - throw 1; - }, int); - - ASSERT_EQ(pool.count(), 0); - } }