From fe7e924026cb2798afd4b7dfee4be1bdbd6fc066 Mon Sep 17 00:00:00 2001
From: eldritch horrors <pennae@lix.systems>
Date: Thu, 21 Mar 2024 21:27:07 +0100
Subject: [PATCH] libutil: drop Pool resources on exceptional free

if a scope owning a resource does not gracefully drop that resource
while handling exceptions from deeper down the call stack we should
assume the resource is invalid state and drop it. currently it *is*
true that such cases do not cause resources to be freed, but thanks
to validator misuses this has so far not caused any larger problem.

Change-Id: Ie4f91bcd60a64d05c5ff9d22cc97954816d13b97
---
 src/libutil/pool.hh        |  9 ++++++++-
 tests/unit/libutil/pool.cc | 15 +++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/src/libutil/pool.hh b/src/libutil/pool.hh
index 6247b6125..548e7ce69 100644
--- a/src/libutil/pool.hh
+++ b/src/libutil/pool.hh
@@ -1,6 +1,7 @@
 #pragma once
 ///@file
 
+#include <exception>
 #include <functional>
 #include <limits>
 #include <list>
@@ -118,7 +119,7 @@ public:
             if (!r) return;
             {
                 auto state_(pool.state.lock());
-                if (!bad)
+                if (!bad && !std::uncaught_exceptions())
                     state_->idle.push_back(ref<R>(r));
                 assert(state_->inUse);
                 state_->inUse--;
@@ -134,6 +135,12 @@ 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 127e42dda..a3743e601 100644
--- a/tests/unit/libutil/pool.cc
+++ b/tests/unit/libutil/pool.cc
@@ -124,4 +124,19 @@ namespace nix {
             ASSERT_NE(h->num, counter);
         }
     }
+
+    TEST(Pool, throwingOperationDropsResource)
+    {
+        auto createResource = []() { return make_ref<TestResource>(); };
+
+        Pool<TestResource> pool = Pool<TestResource>((size_t)1, createResource);
+
+        ASSERT_THROW({
+            auto _r = pool.get();
+            ASSERT_EQ(pool.count(), 1);
+            throw 1;
+        }, int);
+
+        ASSERT_EQ(pool.count(), 0);
+    }
 }