diff --git a/CHANGELOG.md b/CHANGELOG.md index d37108a9684..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] 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 432f6d1a534..d454a76fa1d 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 @@ -738,9 +741,16 @@ namespace asynchost } }; + // 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) @@ -779,6 +789,38 @@ 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 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; + + // 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. + std::shared_ptr async_read_state = + std::make_shared(); + [[nodiscard]] auto get_it_contains_idx(size_t idx) const { if (idx == 0) @@ -1255,6 +1297,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 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 +1340,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 +1736,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,7 +1754,67 @@ namespace asynchost { auto* data = static_cast(req->data); - data->read_result = data->ledger->read_entries_range( + 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(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; + } + + if (async_state->test_worker_active_hook) + { + async_state->test_worker_active_hook(); + } + + data->read_result = ledger->read_entries_range( data->from_idx, data->to_idx, true, data->max_size); } @@ -1817,6 +1940,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, diff --git a/src/host/test/ledger.cpp b/src/host/test/ledger.cpp index 83f3abe923b..8623c239472 100644 --- a/src/host/test/ledger.cpp +++ b/src/host/test/ledger.cpp @@ -1,5 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the Apache 2.0 License. + #include "host/ledger.h" #include "ccf/crypto/sha256_hash.h" @@ -13,9 +14,12 @@ #include "snapshots/snapshot_manager.h" #define DOCTEST_CONFIG_IMPLEMENT +#include #include +#include #include #include +#include using namespace asynchost; @@ -2148,6 +2152,232 @@ 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), 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; + 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); + } +}; + +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(); + std::promise destroyed; + std::thread destroyer; + bool worker_released = false; + + AsyncShutdownTestCleanup cleanup{release_worker, worker_released, destroyer}; + + 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. + 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(); + + // Destroy the Ledger on another thread. ~Ledger must block until the + // in-flight worker finishes, so this must not complete yet. + destroyer = std::thread([&]() { + 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. The cleanup guard above joins the destroyer and drains the + // loop. + release_worker.set_value(); + worker_released = true; + REQUIRE( + destroyed_future.wait_for(std::chrono::seconds(5)) == + 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}; + + LedgerAsyncTestAccess::set_worker_active_hook(*raw_ledger, [&]() { + worker_active.set_value(); + release_future.wait(); + }); + + LedgerAsyncTestAccess::queue_async_read( + *raw_ledger, 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; + LedgerAsyncTestAccess::queue_async_read( + *raw_ledger, + 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();