Fix use-after-free in async host ledger reads on shutdown

CHANGELOG.md

@@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 - Curl multi client shutdown now aborts queued async requests without performing network I/O, and curl response header capture now enforces default header size and count limits (#8005).
 - Changing recovery members or the recovery threshold, refreshing recovery shares, or rekeying the ledger while the service is recovering now correctly returns an error instead of appearing to succeed. These operations were always potentially unsafe because at-recovery ledger secrets cannot be rekeyed; services with custom constitutions should update their `set_member`, `remove_member`, `set_recovery_threshold`, `trigger_recovery_shares_refresh`, and `trigger_ledger_rekey` actions to reject them while recovering (#7980).
+- Asynchronous ledger reads (used to serve committed entry ranges to the enclave) no longer access the host `Ledger` object after it has been destroyed during shutdown. The `Ledger` now waits for any in-flight read workers to finish, and workers that have not yet started skip accessing it, fixing a potential use-after-free on shutdown (#8003).
 
 ## [7.0.6]
 

CMakeLists.txt

@@ -674,6 +674,7 @@ if(BUILD_TESTS)
       ledger_test
       ${CMAKE_CURRENT_SOURCE_DIR}/src/host/test/ledger.cpp
     )
+    target_link_libraries(ledger_test PRIVATE uv)
 
     add_unit_test(
       files_cleanup_test

src/host/ledger.h

@@ -15,11 +15,14 @@
 #include "ledger_filenames.h"
 #include "time_bound_logger.h"
 
+#include <condition_variable>
 #include <cstdint>
 #include <cstdio>
 #include <filesystem>
 #include <list>
 #include <map>
+#include <memory>
+#include <mutex>
 #include <string>
 #include <sys/types.h>
 #include <uv.h>
@@ -738,6 +741,19 @@ namespace asynchost
     }
   };
 
+#ifdef TEST_MODE_LEDGER_ASYNC_HOOK
+  // Test-only hook (see src/host/test/ledger.cpp). Invoked by an async ledger
+  // read worker once it has registered as an active worker (so that ~Ledger
+  // must wait for it) and immediately before it dereferences the Ledger. Lets
+  // tests deterministically hold a worker in-flight while the owning Ledger is
+  // destroyed, exercising the shutdown coordination in ~Ledger.
+  inline std::function<void()>& ledger_async_worker_active_hook()
+  {
+    static std::function<void()> hook;
+    return hook;
+  }
+#endif
+
   class Ledger
   {
   private:
@@ -779,6 +795,28 @@ namespace asynchost
     // complete
     std::optional<size_t> recovery_start_idx = std::nullopt;
 
+    // Shared state used to coordinate the shutdown of in-flight asynchronous
+    // ledger reads. A single instance is shared (via shared_ptr) between this
+    // Ledger and every outstanding AsyncLedgerGet. Because it is reference
+    // counted it outlives the Ledger, so worker callbacks can safely decide
+    // whether they may access the Ledger even during shutdown.
+    struct AsyncReadState
+    {
+      std::mutex lock;
+      std::condition_variable all_workers_done;
+
+      // Number of worker callbacks currently accessing the Ledger
+      size_t active_workers = 0;
+
+      // Set to false when the owning Ledger is being destroyed, after which
+      // workers must not access it
+      bool ledger_alive = true;
+    };
+
+    // See AsyncReadState, on_ledger_get_async and ~Ledger.
+    std::shared_ptr<AsyncReadState> async_read_state =
+      std::make_shared<AsyncReadState>();
+
     [[nodiscard]] auto get_it_contains_idx(size_t idx) const
     {
       if (idx == 0)
@@ -1255,6 +1293,22 @@ namespace asynchost
 
     Ledger(const Ledger& that) = delete;
 
+    ~Ledger()
+    {
+      // Ensure that no asynchronous ledger read accesses this Ledger after it
+      // has been destroyed. Mark the Ledger as no longer available, then wait
+      // for any in-flight worker callbacks (which may be running on a libuv
+      // threadpool thread) to finish accessing it. Workers that have not yet
+      // started will observe ledger_alive == false and skip accessing the
+      // Ledger entirely. The shared async_read_state outlives this Ledger, so
+      // any remaining completion callbacks remain safe to run afterwards
+      // because they do not dereference this Ledger.
+      std::unique_lock<std::mutex> guard(async_read_state->lock);
+      async_read_state->ledger_alive = false;
+      async_read_state->all_workers_done.wait(
+        guard, [this]() { return async_read_state->active_workers == 0; });
+    }
+
     void init(size_t idx, size_t recovery_start_idx_ = 0)
     {
       TimeBoundLogger log_if_slow(
@@ -1282,7 +1336,8 @@ namespace asynchost
           if (!last_idx_file.has_value())
           {
             throw std::logic_error(fmt::format(
-              "Committed ledger file {} does not include last idx in file name",
+              "Committed ledger file {} does not include last idx in file "
+              "name",
               file_name));
           }
 
@@ -1677,6 +1732,10 @@ namespace asynchost
       size_t to_idx{};
       size_t max_size{};
 
+      // Shared with the owning Ledger to coordinate safe shutdown, so that the
+      // worker callback never accesses a destroyed Ledger
+      std::shared_ptr<AsyncReadState> async_state;
+
       // First argument is ledger entries (or nullopt if not found)
       // Second argument is uv status code, which may indicate a cancellation
       using ResultCallback =
@@ -1691,7 +1750,69 @@ namespace asynchost
     {
       auto* data = static_cast<AsyncLedgerGet*>(req->data);
 
-      data->read_result = data->ledger->read_entries_range(
+      struct ActiveWorkerGuard
+      {
+        std::shared_ptr<AsyncReadState> async_state;
+        bool active = false;
+
+        ActiveWorkerGuard(std::shared_ptr<AsyncReadState> async_state_) :
+          async_state(std::move(async_state_))
+        {}
+
+        void activate()
+        {
+          active = true;
+        }
+
+        ~ActiveWorkerGuard()
+        {
+          if (active)
+          {
+            std::unique_lock<std::mutex> guard(async_state->lock);
+            --async_state->active_workers;
+            async_state->all_workers_done.notify_all();
+          }
+        }
+      };
+
+      auto async_state = data->async_state;
+      ActiveWorkerGuard active_worker(async_state);
+      Ledger* ledger = nullptr;
+      {
+        std::unique_lock<std::mutex> guard(async_state->lock);
+        if (!async_state->ledger_alive)
+        {
+          // The Ledger has been (or is being) destroyed, so it must not be
+          // accessed. Leave read_result empty.
+          LOG_DEBUG_FMT(
+            "Skipping async ledger read {} to {} because Ledger is shutting "
+            "down",
+            data->from_idx,
+            data->to_idx);
+          return;
+        }
+
+        // Register as an active worker so that ~Ledger waits for this callback
+        // to finish before the Ledger is destroyed. Because we observed
+        // ledger_alive == true while holding the lock and incremented
+        // active_workers, ~Ledger cannot complete (and therefore cannot start
+        // destroying the Ledger's members) until active_worker has decremented
+        // it.
+        // This makes it safe to dereference the raw ledger pointer outside the
+        // lock.
+        ++async_state->active_workers;
+        active_worker.activate();
+        ledger = data->ledger;
+      }
+
+#ifdef TEST_MODE_LEDGER_ASYNC_HOOK
+      if (ledger_async_worker_active_hook())
+      {
+        ledger_async_worker_active_hook()();
+      }
+#endif
+
+      data->read_result = ledger->read_entries_range(
         data->from_idx, data->to_idx, true, data->max_size);
     }
 
@@ -1705,6 +1826,47 @@ namespace asynchost
       delete req; // NOLINT(cppcoreguidelines-owning-memory)
     }
 
+#ifdef TEST_MODE_LEDGER_ASYNC_HOOK
+    // Test-only: queue an asynchronous committed-range read using the same
+    // worker and completion callbacks as the ledger_get_range handler, without
+    // requiring the ringbuffer message plumbing. Used to exercise the shutdown
+    // coordination between in-flight async reads and ~Ledger. The optional
+    // result callback lets a test observe the read outcome (e.g. to confirm a
+    // read was skipped rather than served).
+    void test_queue_async_read(
+      size_t from_idx,
+      size_t to_idx,
+      AsyncLedgerGet::ResultCallback result_cb =
+        [](std::optional<LedgerReadResult>&&, int) {})
+    {
+      // NOLINTNEXTLINE(cppcoreguidelines-owning-memory)
+      auto* work_handle = new uv_work_t;
+
+      // NOLINTNEXTLINE(cppcoreguidelines-owning-memory)
+      auto* job = new AsyncLedgerGet;
+      job->ledger = this;
+      job->from_idx = from_idx;
+      job->to_idx = to_idx;
+      job->max_size = SIZE_MAX;
+      job->async_state = async_read_state;
+      job->result_cb = std::move(result_cb);
+      work_handle->data = job;
+
+      int rc = uv_queue_work(
+        uv_default_loop(),
+        work_handle,
+        &on_ledger_get_async,
+        &on_ledger_get_async_complete);
+      if (rc < 0)
+      {
+        delete job; // NOLINT(cppcoreguidelines-owning-memory)
+        delete work_handle; // NOLINT(cppcoreguidelines-owning-memory)
+        throw std::logic_error(fmt::format(
+          "Failed to queue test async ledger read: {}", uv_strerror(rc)));
+      }
+    }
+#endif
+
     static void write_ledger_get_range_response(
       const ringbuffer::WriterPtr& to_enclave_,
       size_t from_idx,
@@ -1817,6 +1979,7 @@ namespace asynchost
               job->from_idx = from_idx;
               job->to_idx = to_idx;
               job->max_size = max_entries_size;
+              job->async_state = async_read_state;
               job->result_cb = [to_enclave_ = to_enclave,
                                 from_idx_ = from_idx,
                                 to_idx_ = to_idx,