From 6ba0d626164545fd4df395dde599de2263442ce7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 29 Jun 2026 17:20:55 +0000 Subject: [PATCH 1/9] Initial plan From 69657fac8c80f497e5783674f478c8ec5d48717e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 29 Jun 2026 17:32:11 +0000 Subject: [PATCH 2/9] Fix use-after-free in async ledger reads on shutdown Closes #3501 Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com> --- CHANGELOG.md | 1 + src/host/ledger.h | 74 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e2ef85f32c..a3ecbb2d34a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed - Forwarded commands are no longer processed until the node is part of the network, matching the existing behaviour for other node-to-node messages. Previously a forwarded command could be executed while the node was in an earlier startup state, which could lead to undefined behaviour for some commands (#7936). +- 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.5] diff --git a/src/host/ledger.h b/src/host/ledger.h index 432f6d1a534..b9d35438d69 100644 --- a/src/host/ledger.h +++ b/src/host/ledger.h @@ -15,11 +15,14 @@ #include "ledger_filenames.h" #include "time_bound_logger.h" +#include #include #include #include #include #include +#include +#include #include #include #include @@ -779,6 +782,31 @@ namespace asynchost // complete std::optional 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 the worker and completion callbacks + // can run safely even if the Ledger has already been destroyed. + 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; + }; + + // Shared state used to coordinate the shutdown of in-flight asynchronous + // ledger reads (see on_ledger_get_async and ~Ledger). It is held by a + // shared_ptr so that it outlives this Ledger, allowing worker and + // completion callbacks to run safely even after the Ledger is destroyed. + std::shared_ptr async_read_state = + std::make_shared(); + [[nodiscard]] auto get_it_contains_idx(size_t idx) const { if (idx == 0) @@ -1255,6 +1283,21 @@ 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. + std::unique_lock 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( @@ -1677,6 +1720,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 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,8 +1738,30 @@ namespace asynchost { auto* data = static_cast(req->data); - data->read_result = data->ledger->read_entries_range( - data->from_idx, data->to_idx, true, data->max_size); + Ledger* ledger = nullptr; + { + std::unique_lock guard(data->async_state->lock); + if (!data->async_state->ledger_alive) + { + // The Ledger has been (or is being) destroyed, so it must not be + // accessed. Leave read_result empty. + return; + } + + // Register as an active worker so that ~Ledger waits for this callback + // to finish before the Ledger is destroyed. + ++data->async_state->active_workers; + ledger = data->ledger; + } + + data->read_result = + ledger->read_entries_range(data->from_idx, data->to_idx, true, data->max_size); + + { + std::unique_lock guard(data->async_state->lock); + --data->async_state->active_workers; + data->async_state->all_workers_done.notify_all(); + } } static void on_ledger_get_async_complete(uv_work_t* req, int status) @@ -1817,6 +1886,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, From 4c46e413fd9a535927339cc568c2422a75cfa771 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 29 Jun 2026 17:34:22 +0000 Subject: [PATCH 3/9] Address review: log skipped async reads and clarify lifetime safety Closes #3501 Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com> --- src/host/ledger.h | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/host/ledger.h b/src/host/ledger.h index b9d35438d69..1a462a37a38 100644 --- a/src/host/ledger.h +++ b/src/host/ledger.h @@ -1745,17 +1745,27 @@ namespace asynchost { // 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. + // 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 we have decremented it below. + // This makes it safe to dereference the raw ledger pointer outside the + // lock. ++data->async_state->active_workers; ledger = data->ledger; } - data->read_result = - ledger->read_entries_range(data->from_idx, data->to_idx, true, data->max_size); + data->read_result = ledger->read_entries_range( + data->from_idx, data->to_idx, true, data->max_size); { std::unique_lock guard(data->async_state->lock); From f527f01141954c007ddc71753adef2cc63902f2a Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Tue, 30 Jun 2026 21:05:26 +0000 Subject: [PATCH 4/9] Address async ledger read review comments Make the active worker accounting exception-safe and clarify that AsyncReadState protects worker access to Ledger while completion callbacks are Ledger-independent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/host/ledger.h | 54 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 39 insertions(+), 15 deletions(-) diff --git a/src/host/ledger.h b/src/host/ledger.h index 1a462a37a38..ed2e9cd5ef0 100644 --- a/src/host/ledger.h +++ b/src/host/ledger.h @@ -785,8 +785,8 @@ namespace asynchost // 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 the worker and completion callbacks - // can run safely even if the Ledger has already been destroyed. + // 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; @@ -802,8 +802,8 @@ namespace asynchost // Shared state used to coordinate the shutdown of in-flight asynchronous // ledger reads (see on_ledger_get_async and ~Ledger). It is held by a - // shared_ptr so that it outlives this Ledger, allowing worker and - // completion callbacks to run safely even after the Ledger is destroyed. + // shared_ptr so that worker callbacks can still consult it after this + // Ledger starts being destroyed. std::shared_ptr async_read_state = std::make_shared(); @@ -1291,7 +1291,8 @@ namespace asynchost // 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. + // any remaining completion callbacks remain safe to run afterwards + // because they do not dereference this Ledger. std::unique_lock guard(async_read_state->lock); async_read_state->ledger_alive = false; async_read_state->all_workers_done.wait( @@ -1738,10 +1739,37 @@ namespace asynchost { auto* data = static_cast(req->data); + struct ActiveWorkerGuard + { + std::shared_ptr async_state; + bool active = false; + + ActiveWorkerGuard(std::shared_ptr async_state_) : + async_state(std::move(async_state_)) + {} + + void activate() + { + active = true; + } + + ~ActiveWorkerGuard() + { + if (active) + { + std::unique_lock 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 guard(data->async_state->lock); - if (!data->async_state->ledger_alive) + std::unique_lock 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. @@ -1757,21 +1785,17 @@ namespace asynchost // 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 we have decremented it below. + // destroying the Ledger's members) until active_worker has decremented + // it. // This makes it safe to dereference the raw ledger pointer outside the // lock. - ++data->async_state->active_workers; + ++async_state->active_workers; + active_worker.activate(); ledger = data->ledger; } data->read_result = ledger->read_entries_range( data->from_idx, data->to_idx, true, data->max_size); - - { - std::unique_lock guard(data->async_state->lock); - --data->async_state->active_workers; - data->async_state->all_workers_done.notify_all(); - } } static void on_ledger_get_async_complete(uv_work_t* req, int status) From 98d97f5746a4756d2ec06dfb641120ff456a319c Mon Sep 17 00:00:00 2001 From: achamayou <4016369+achamayou@users.noreply.github.com> Date: Fri, 3 Jul 2026 14:31:38 +0000 Subject: [PATCH 5/9] Add deterministic test for async ledger read shutdown coordination Adds a ledger_test case that queues an in-flight async committed-range read, holds it active on a libuv threadpool thread, then destroys the Ledger on another thread and asserts ~Ledger blocks until the worker completes - exercising the use-after-free fix from #8003. Validated under ASAN/UBSan. - ledger.h: add guarded (TEST_MODE_LEDGER_ASYNC_HOOK) worker hook and test_queue_async_read trigger; tidy async_read_state comment - ledger_test: new test case; link target against libuv - CHANGELOG: move the #8003 entry to the current 7.0.7 section --- CHANGELOG.md | 2 +- CMakeLists.txt | 1 + src/host/ledger.h | 53 ++++++++++++++++++++++++++++--- src/host/test/ledger.cpp | 68 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 119 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3dab0cb1ac..63f4380d155 100644 --- a/CHANGELOG.md +++ b/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] @@ -31,7 +32,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed - Forwarded commands are no longer processed until the node is part of the network, matching the existing behaviour for other node-to-node messages. Previously a forwarded command could be executed while the node was in an earlier startup state, which could lead to undefined behaviour for some commands (#7936). -- 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.5] diff --git a/CMakeLists.txt b/CMakeLists.txt index 692077e6465..d7b96a6676b 100644 --- a/CMakeLists.txt +++ b/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 diff --git a/src/host/ledger.h b/src/host/ledger.h index ed2e9cd5ef0..1b7427f0c7b 100644 --- a/src/host/ledger.h +++ b/src/host/ledger.h @@ -741,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& ledger_async_worker_active_hook() + { + static std::function hook; + return hook; + } +#endif + class Ledger { private: @@ -800,10 +813,7 @@ namespace asynchost bool ledger_alive = true; }; - // Shared state used to coordinate the shutdown of in-flight asynchronous - // ledger reads (see on_ledger_get_async and ~Ledger). It is held by a - // shared_ptr so that worker callbacks can still consult it after this - // Ledger starts being destroyed. + // See AsyncReadState, on_ledger_get_async and ~Ledger. std::shared_ptr async_read_state = std::make_shared(); @@ -1794,6 +1804,13 @@ namespace asynchost 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); } @@ -1808,6 +1825,34 @@ 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. + void test_queue_async_read(size_t from_idx, size_t to_idx) + { + // 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::optional&&, int) {}; + work_handle->data = job; + } + uv_queue_work( + uv_default_loop(), + work_handle, + &on_ledger_get_async, + &on_ledger_get_async_complete); + } +#endif + static void write_ledger_get_range_response( const ringbuffer::WriterPtr& to_enclave_, size_t from_idx, diff --git a/src/host/test/ledger.cpp b/src/host/test/ledger.cpp index 83f3abe923b..00f4ad48ee9 100644 --- a/src/host/test/ledger.cpp +++ b/src/host/test/ledger.cpp @@ -1,5 +1,8 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the Apache 2.0 License. + +// Enables the test-only async read hook and trigger in host/ledger.h +#define TEST_MODE_LEDGER_ASYNC_HOOK #include "host/ledger.h" #include "ccf/crypto/sha256_hash.h" @@ -13,9 +16,12 @@ #include "snapshots/snapshot_manager.h" #define DOCTEST_CONFIG_IMPLEMENT +#include #include +#include #include #include +#include using namespace asynchost; @@ -2148,6 +2154,68 @@ TEST_CASE("Ledger init with existing files") } } +TEST_CASE("Async ledger read blocks Ledger destruction until complete") +{ + auto dir = AutoDeleteFolder(ledger_dir); + + const size_t chunk_threshold = 30; + const size_t entries_per_chunk = get_entries_per_chunk(chunk_threshold); + + // Heap-allocate so the Ledger can be destroyed from a separate thread while + // the main thread observes whether the destructor blocks. + auto ledger = std::make_unique(ledger_dir, wf); + TestEntrySubmitter entry_submitter(*ledger, chunk_threshold); + + // Create several chunks and commit the first one, so that a read of that + // range takes the asynchronous (committed-file) path. + const size_t end_of_first_chunk_idx = + initialise_ledger(entry_submitter, entries_per_chunk, 3); + ledger->commit(end_of_first_chunk_idx); + REQUIRE(ledger->is_in_committed_file(end_of_first_chunk_idx)); + + // The worker signals when it has registered as active (so ~Ledger must wait + // for it), then blocks until the test releases it. + std::promise worker_active; + std::promise release_worker; + auto release_future = release_worker.get_future(); + asynchost::ledger_async_worker_active_hook() = [&]() { + worker_active.set_value(); + release_future.wait(); + }; + + // Queue the asynchronous read. The worker runs on a libuv threadpool thread. + ledger->test_queue_async_read(1, end_of_first_chunk_idx); + + // Wait until the worker is in-flight and holding the Ledger alive. + worker_active.get_future().wait(); + + // Destroy the Ledger on another thread. ~Ledger must block until the + // in-flight worker finishes, so this must not complete yet. + std::promise destroyed; + std::thread destroyer([&]() { + ledger.reset(); + destroyed.set_value(); + }); + auto destroyed_future = destroyed.get_future(); + + REQUIRE( + destroyed_future.wait_for(std::chrono::milliseconds(500)) == + std::future_status::timeout); + + // Release the worker; the read completes on a live Ledger and ~Ledger can + // now finish. + release_worker.set_value(); + REQUIRE( + destroyed_future.wait_for(std::chrono::seconds(5)) == + std::future_status::ready); + destroyer.join(); + + // Drain the completion callback so the AsyncLedgerGet/uv_work_t are freed. + uv_run(uv_default_loop(), UV_RUN_DEFAULT); + + asynchost::ledger_async_worker_active_hook() = nullptr; +} + int main(int argc, char** argv) { ccf::logger::config::default_init(); From 3f3eb8be2abed2f4669bb03878b254f1ae6e04be Mon Sep 17 00:00:00 2001 From: Amaury Chamayou Date: Fri, 3 Jul 2026 15:40:59 +0100 Subject: [PATCH 6/9] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/host/ledger.h | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/src/host/ledger.h b/src/host/ledger.h index 1b7427f0c7b..67df7aba655 100644 --- a/src/host/ledger.h +++ b/src/host/ledger.h @@ -1834,22 +1834,29 @@ namespace asynchost { // 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::optional&&, int) {}; - work_handle->data = job; - } - uv_queue_work( + + // 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::optional&&, int) {}; + 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 From 2ac7c730442767dff366453188560563a537a35b Mon Sep 17 00:00:00 2001 From: achamayou <4016369+achamayou@users.noreply.github.com> Date: Fri, 3 Jul 2026 14:50:37 +0000 Subject: [PATCH 7/9] Make async ledger read shutdown test cleanup exception-safe Replace the manual hook/thread teardown with a single RAII guard that, on every exit path (including a failed REQUIRE, which doctest throws), releases the in-flight worker so ~Ledger can finish, joins the destroyer thread (a joinable std::thread must never be destroyed), drains the completion callback, and clears the global worker hook so it can never retain dangling references to the test frame. --- src/host/test/ledger.cpp | 47 ++++++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/src/host/test/ledger.cpp b/src/host/test/ledger.cpp index 00f4ad48ee9..0202a6f474c 100644 --- a/src/host/test/ledger.cpp +++ b/src/host/test/ledger.cpp @@ -2178,6 +2178,40 @@ TEST_CASE("Async ledger read blocks Ledger destruction until complete") std::promise worker_active; std::promise release_worker; auto release_future = release_worker.get_future(); + std::promise destroyed; + std::thread destroyer; + bool worker_released = false; + + // RAII cleanup that runs on every exit path, including a failed REQUIRE + // (doctest throws on assertion failure). It releases the in-flight worker so + // ~Ledger can finish, joins the destroyer thread (a joinable std::thread must + // never be destroyed), drains the completion callback, and clears the global + // hook so it cannot retain dangling references to this stack frame. Declared + // before the hook and thread are set up so it is destroyed first, while + // everything it references is still alive. + struct HookAndThreadCleanup + { + std::promise& release_worker_promise; + bool& worker_was_released; + std::thread& destroyer_thread; + + ~HookAndThreadCleanup() + { + if (!worker_was_released) + { + release_worker_promise.set_value(); + } + if (destroyer_thread.joinable()) + { + destroyer_thread.join(); + } + // Drain the completion callback so the AsyncLedgerGet/uv_work_t are + // freed. + uv_run(uv_default_loop(), UV_RUN_DEFAULT); + asynchost::ledger_async_worker_active_hook() = nullptr; + } + } cleanup{release_worker, worker_released, destroyer}; + asynchost::ledger_async_worker_active_hook() = [&]() { worker_active.set_value(); release_future.wait(); @@ -2191,8 +2225,7 @@ TEST_CASE("Async ledger read blocks Ledger destruction until complete") // Destroy the Ledger on another thread. ~Ledger must block until the // in-flight worker finishes, so this must not complete yet. - std::promise destroyed; - std::thread destroyer([&]() { + destroyer = std::thread([&]() { ledger.reset(); destroyed.set_value(); }); @@ -2203,17 +2236,13 @@ TEST_CASE("Async ledger read blocks Ledger destruction until complete") std::future_status::timeout); // Release the worker; the read completes on a live Ledger and ~Ledger can - // now finish. + // now finish. The cleanup guard above joins the destroyer, drains the loop, + // and clears the hook. release_worker.set_value(); + worker_released = true; REQUIRE( destroyed_future.wait_for(std::chrono::seconds(5)) == std::future_status::ready); - destroyer.join(); - - // Drain the completion callback so the AsyncLedgerGet/uv_work_t are freed. - uv_run(uv_default_loop(), UV_RUN_DEFAULT); - - asynchost::ledger_async_worker_active_hook() = nullptr; } int main(int argc, char** argv) From 87839c4fc141c12ab149a90f97af975d897f6134 Mon Sep 17 00:00:00 2001 From: achamayou <4016369+achamayou@users.noreply.github.com> Date: Fri, 3 Jul 2026 15:07:03 +0000 Subject: [PATCH 8/9] Add skip-path coverage for async ledger read shutdown test_queue_async_read now takes an optional result callback so a test can observe the read outcome. Adds a test that queues a read after ~Ledger has begun (ledger_alive == false) and asserts it is skipped rather than served, and extracts the shutdown cleanup into a shared helper reused by both async shutdown tests. --- src/host/ledger.h | 15 +++-- src/host/test/ledger.cpp | 137 ++++++++++++++++++++++++++++++--------- 2 files changed, 119 insertions(+), 33 deletions(-) diff --git a/src/host/ledger.h b/src/host/ledger.h index 67df7aba655..764c28f4820 100644 --- a/src/host/ledger.h +++ b/src/host/ledger.h @@ -1336,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)); } @@ -1829,8 +1830,14 @@ namespace asynchost // 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. - void test_queue_async_read(size_t from_idx, size_t to_idx) + // 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&&, int) {}) { // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) auto* work_handle = new uv_work_t; @@ -1842,7 +1849,7 @@ namespace asynchost job->to_idx = to_idx; job->max_size = SIZE_MAX; job->async_state = async_read_state; - job->result_cb = [](std::optional&&, int) {}; + job->result_cb = std::move(result_cb); work_handle->data = job; int rc = uv_queue_work( diff --git a/src/host/test/ledger.cpp b/src/host/test/ledger.cpp index 0202a6f474c..c07c4df797e 100644 --- a/src/host/test/ledger.cpp +++ b/src/host/test/ledger.cpp @@ -2154,6 +2154,35 @@ TEST_CASE("Ledger init with existing files") } } +// Test helper: on destruction - which happens on every exit path, including a +// failed REQUIRE that doctest throws - releases a held in-flight async read +// worker so ~Ledger can finish, joins the thread that destroys the Ledger (a +// joinable std::thread must never be destroyed), drains the libuv completion +// callbacks, and clears the global worker hook so it cannot retain dangling +// references to the test's stack frame. Declare it before the hook and thread +// are set up so it is destroyed first, while everything it references is still +// alive. +struct AsyncShutdownTestCleanup +{ + std::promise& release_worker; + bool& worker_released; + std::thread& destroyer; + + ~AsyncShutdownTestCleanup() + { + if (!worker_released) + { + release_worker.set_value(); + } + if (destroyer.joinable()) + { + destroyer.join(); + } + uv_run(uv_default_loop(), UV_RUN_DEFAULT); + asynchost::ledger_async_worker_active_hook() = nullptr; + } +}; + TEST_CASE("Async ledger read blocks Ledger destruction until complete") { auto dir = AutoDeleteFolder(ledger_dir); @@ -2182,35 +2211,7 @@ TEST_CASE("Async ledger read blocks Ledger destruction until complete") std::thread destroyer; bool worker_released = false; - // RAII cleanup that runs on every exit path, including a failed REQUIRE - // (doctest throws on assertion failure). It releases the in-flight worker so - // ~Ledger can finish, joins the destroyer thread (a joinable std::thread must - // never be destroyed), drains the completion callback, and clears the global - // hook so it cannot retain dangling references to this stack frame. Declared - // before the hook and thread are set up so it is destroyed first, while - // everything it references is still alive. - struct HookAndThreadCleanup - { - std::promise& release_worker_promise; - bool& worker_was_released; - std::thread& destroyer_thread; - - ~HookAndThreadCleanup() - { - if (!worker_was_released) - { - release_worker_promise.set_value(); - } - if (destroyer_thread.joinable()) - { - destroyer_thread.join(); - } - // Drain the completion callback so the AsyncLedgerGet/uv_work_t are - // freed. - uv_run(uv_default_loop(), UV_RUN_DEFAULT); - asynchost::ledger_async_worker_active_hook() = nullptr; - } - } cleanup{release_worker, worker_released, destroyer}; + AsyncShutdownTestCleanup cleanup{release_worker, worker_released, destroyer}; asynchost::ledger_async_worker_active_hook() = [&]() { worker_active.set_value(); @@ -2245,6 +2246,84 @@ TEST_CASE("Async ledger read blocks Ledger destruction until complete") std::future_status::ready); } +TEST_CASE("Async ledger read started after shutdown begins is skipped") +{ + auto dir = AutoDeleteFolder(ledger_dir); + + const size_t chunk_threshold = 30; + const size_t entries_per_chunk = get_entries_per_chunk(chunk_threshold); + + auto ledger = std::make_unique(ledger_dir, wf); + TestEntrySubmitter entry_submitter(*ledger, chunk_threshold); + + const size_t end_of_first_chunk_idx = + initialise_ledger(entry_submitter, entries_per_chunk, 3); + ledger->commit(end_of_first_chunk_idx); + REQUIRE(ledger->is_in_committed_file(end_of_first_chunk_idx)); + + // Raw pointer so a second read can be queued while ~Ledger is running: the + // destructor blocks before destroying any members, so the object is still + // usable, but the owning unique_ptr is already null once reset() is entered. + Ledger* raw_ledger = ledger.get(); + + // Worker #1 is pinned active so that ~Ledger sets ledger_alive = false and + // then blocks, giving us a window in which ledger_alive == false. + std::promise worker_active; + std::promise release_worker; + auto release_future = release_worker.get_future(); + std::promise destroyed; + std::thread destroyer; + bool worker_released = false; + + AsyncShutdownTestCleanup cleanup{release_worker, worker_released, destroyer}; + + asynchost::ledger_async_worker_active_hook() = [&]() { + worker_active.set_value(); + release_future.wait(); + }; + + raw_ledger->test_queue_async_read(1, end_of_first_chunk_idx); + worker_active.get_future().wait(); + + destroyer = std::thread([&]() { + ledger.reset(); + destroyed.set_value(); + }); + auto destroyed_future = destroyed.get_future(); + + // Confirm ~Ledger is blocked: at this point ledger_alive is already false. + REQUIRE( + destroyed_future.wait_for(std::chrono::milliseconds(500)) == + std::future_status::timeout); + + // Queue a second read now, while ledger_alive == false. It must take the + // skip branch rather than accessing the shutting-down Ledger. A skipped read + // reports no value; an alive read of this valid committed range would report + // one. + std::promise second_read_served; + raw_ledger->test_queue_async_read( + 1, + end_of_first_chunk_idx, + [&](std::optional&& result, int) { + second_read_served.set_value(result.has_value()); + }); + + // Release worker #1 so ~Ledger can complete. + release_worker.set_value(); + worker_released = true; + REQUIRE( + destroyed_future.wait_for(std::chrono::seconds(5)) == + std::future_status::ready); + + // Drain both completion callbacks (worker #1 served, worker #2 skipped). + uv_run(uv_default_loop(), UV_RUN_DEFAULT); + + auto served = second_read_served.get_future(); + REQUIRE( + served.wait_for(std::chrono::seconds(0)) == std::future_status::ready); + REQUIRE(served.get() == false); +} + int main(int argc, char** argv) { ccf::logger::config::default_init(); From c0ccaa710190dd803b4a3dbb32214554cc0da432 Mon Sep 17 00:00:00 2001 From: achamayou <4016369+achamayou@users.noreply.github.com> Date: Mon, 6 Jul 2026 09:32:27 +0000 Subject: [PATCH 9/9] Replace #ifdef-gated test hook with friend-based test accessor Address review feedback on PR #8003: avoid putting test-only code behind an #ifdef in the production header. Replace TEST_MODE_LEDGER_ASYNC_HOOK (a global mutable hook function and a public test_queue_async_read method) with: - An always-compiled, no-op-by-default test_worker_active_hook field on the private AsyncReadState struct, called unconditionally in on_ledger_get_async. - A forward-declared LedgerAsyncTestAccess struct, befriended by Ledger, defined only in src/host/test/ledger.cpp, which uses that friendship to reach the private async_read_state member and queue async reads via the already-public AsyncLedgerGet, on_ledger_get_async and on_ledger_get_async_complete. No behaviour change in production builds; the two async shutdown tests are updated to use the new accessor. --- src/host/ledger.h | 75 ++++++++-------------------------- src/host/test/ledger.cpp | 88 ++++++++++++++++++++++++++++++++-------- 2 files changed, 89 insertions(+), 74 deletions(-) diff --git a/src/host/ledger.h b/src/host/ledger.h index 764c28f4820..d454a76fa1d 100644 --- a/src/host/ledger.h +++ b/src/host/ledger.h @@ -741,22 +741,16 @@ 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& ledger_async_worker_active_hook() - { - static std::function hook; - return hook; - } -#endif + // Test-only accessor (see src/host/test/ledger.cpp), befriended below so + // that tests can exercise the async read shutdown-coordination path + // directly, without adding any test-only members to Ledger's public API. + struct LedgerAsyncTestAccess; class Ledger { private: + friend struct LedgerAsyncTestAccess; + ringbuffer::WriterPtr to_enclave; // Main ledger directory (write and read) @@ -811,6 +805,16 @@ namespace asynchost // Set to false when the owning Ledger is being destroyed, after which // workers must not access it bool ledger_alive = true; + + // Test-only hook (see LedgerAsyncTestAccess in 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. Always present (so + // on_ledger_get_async contains no test-only #ifdef), but a no-op unless + // set by a test. + std::function test_worker_active_hook; }; // See AsyncReadState, on_ledger_get_async and ~Ledger. @@ -1805,12 +1809,10 @@ namespace asynchost ledger = data->ledger; } -#ifdef TEST_MODE_LEDGER_ASYNC_HOOK - if (ledger_async_worker_active_hook()) + if (async_state->test_worker_active_hook) { - ledger_async_worker_active_hook()(); + async_state->test_worker_active_hook(); } -#endif data->read_result = ledger->read_entries_range( data->from_idx, data->to_idx, true, data->max_size); @@ -1826,47 +1828,6 @@ 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&&, 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, diff --git a/src/host/test/ledger.cpp b/src/host/test/ledger.cpp index c07c4df797e..8623c239472 100644 --- a/src/host/test/ledger.cpp +++ b/src/host/test/ledger.cpp @@ -1,8 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the Apache 2.0 License. -// Enables the test-only async read hook and trigger in host/ledger.h -#define TEST_MODE_LEDGER_ASYNC_HOOK #include "host/ledger.h" #include "ccf/crypto/sha256_hash.h" @@ -2154,14 +2152,69 @@ TEST_CASE("Ledger init with existing files") } } +namespace asynchost +{ + // Test-only accessor, befriended by Ledger (see host/ledger.h), granting + // access to the internals needed to exercise the async read shutdown + // coordination path directly - without requiring any test-only members on + // Ledger's public API. + struct LedgerAsyncTestAccess + { + // Sets the per-instance hook invoked by an async read worker once it has + // registered as active (see AsyncReadState::test_worker_active_hook). + static void set_worker_active_hook( + Ledger& ledger, std::function hook) + { + ledger.async_read_state->test_worker_active_hook = std::move(hook); + } + + // 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. The optional result + // callback lets a test observe the read outcome (e.g. to confirm a read + // was skipped rather than served). + static void queue_async_read( + Ledger& ledger, + size_t from_idx, + size_t to_idx, + Ledger::AsyncLedgerGet::ResultCallback result_cb = + [](std::optional&&, int) {}) + { + // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) + auto* work_handle = new uv_work_t; + + // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) + auto* job = new Ledger::AsyncLedgerGet; + job->ledger = &ledger; + job->from_idx = from_idx; + job->to_idx = to_idx; + job->max_size = SIZE_MAX; + job->async_state = ledger.async_read_state; + job->result_cb = std::move(result_cb); + work_handle->data = job; + + int rc = uv_queue_work( + uv_default_loop(), + work_handle, + &Ledger::on_ledger_get_async, + &Ledger::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))); + } + } + }; +} + // Test helper: on destruction - which happens on every exit path, including a // failed REQUIRE that doctest throws - releases a held in-flight async read // worker so ~Ledger can finish, joins the thread that destroys the Ledger (a -// joinable std::thread must never be destroyed), drains the libuv completion -// callbacks, and clears the global worker hook so it cannot retain dangling -// references to the test's stack frame. Declare it before the hook and thread -// are set up so it is destroyed first, while everything it references is still -// alive. +// joinable std::thread must never be destroyed), and drains the libuv +// completion callbacks. Declare it before the hook and thread are set up so +// it is destroyed first, while everything it references is still alive. struct AsyncShutdownTestCleanup { std::promise& release_worker; @@ -2179,7 +2232,6 @@ struct AsyncShutdownTestCleanup destroyer.join(); } uv_run(uv_default_loop(), UV_RUN_DEFAULT); - asynchost::ledger_async_worker_active_hook() = nullptr; } }; @@ -2213,13 +2265,13 @@ TEST_CASE("Async ledger read blocks Ledger destruction until complete") AsyncShutdownTestCleanup cleanup{release_worker, worker_released, destroyer}; - asynchost::ledger_async_worker_active_hook() = [&]() { + LedgerAsyncTestAccess::set_worker_active_hook(*ledger, [&]() { worker_active.set_value(); release_future.wait(); - }; + }); // Queue the asynchronous read. The worker runs on a libuv threadpool thread. - ledger->test_queue_async_read(1, end_of_first_chunk_idx); + LedgerAsyncTestAccess::queue_async_read(*ledger, 1, end_of_first_chunk_idx); // Wait until the worker is in-flight and holding the Ledger alive. worker_active.get_future().wait(); @@ -2237,8 +2289,8 @@ TEST_CASE("Async ledger read blocks Ledger destruction until complete") std::future_status::timeout); // Release the worker; the read completes on a live Ledger and ~Ledger can - // now finish. The cleanup guard above joins the destroyer, drains the loop, - // and clears the hook. + // now finish. The cleanup guard above joins the destroyer and drains the + // loop. release_worker.set_value(); worker_released = true; REQUIRE( @@ -2277,12 +2329,13 @@ TEST_CASE("Async ledger read started after shutdown begins is skipped") AsyncShutdownTestCleanup cleanup{release_worker, worker_released, destroyer}; - asynchost::ledger_async_worker_active_hook() = [&]() { + LedgerAsyncTestAccess::set_worker_active_hook(*raw_ledger, [&]() { worker_active.set_value(); release_future.wait(); - }; + }); - raw_ledger->test_queue_async_read(1, end_of_first_chunk_idx); + LedgerAsyncTestAccess::queue_async_read( + *raw_ledger, 1, end_of_first_chunk_idx); worker_active.get_future().wait(); destroyer = std::thread([&]() { @@ -2301,7 +2354,8 @@ TEST_CASE("Async ledger read started after shutdown begins is skipped") // reports no value; an alive read of this valid committed range would report // one. std::promise second_read_served; - raw_ledger->test_queue_async_read( + LedgerAsyncTestAccess::queue_async_read( + *raw_ledger, 1, end_of_first_chunk_idx, [&](std::optional&& result, int) {