Fix use-after-free in async host ledger reads on shutdown#8003
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a shutdown-only use-after-free risk in the host’s async ledger read path (uv_work_t threadpool jobs) by ensuring worker callbacks never dereference a Ledger* after the owning asynchost::Ledger has been destroyed. It does this by introducing a shared control block that outlives the Ledger and by having ~Ledger() wait for in-flight worker callbacks to finish before allowing member destruction.
Changes:
- Added a
shared_ptr-ownedAsyncReadState(mutex + condition variable +active_workers+ledger_alive) shared betweenLedgerand eachAsyncLedgerGet. - Updated
on_ledger_get_asyncto (a) skip reads once shutdown begins and (b) register/deregister active worker callbacks so~Ledger()can safely drain them. - Added a CHANGELOG entry documenting the shutdown use-after-free fix.
Custom instructions used:
.github/copilot-instructions.md.github/instructions/reviewing.instructions.md
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/host/ledger.h |
Introduces AsyncReadState and shutdown coordination so async libuv workers don’t access a destroyed Ledger during/after shutdown. |
CHANGELOG.md |
Documents the shutdown UAF fix with a PR reference. |
|
No blocking issues found. I reviewed PR #8003 against main, focusing on Correctness:
Non-blocking suggestions:
CI note: At the time of review, the main VM/ACI checks for the current head were still queued or in progress; ASAN/TSAN and long jobs were skipped by workflow configuration. Custom instructions used:
|
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>
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
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
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.
| #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 |
There was a problem hiding this comment.
This pattern (putting test code in the implementation header, gated behind an #ifdef) is not something we've done anywhere else. I think it's a risky pattern, and would prefer if we could avoid it.
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.
Async
uv_work_tledger reads insrc/host/ledger.hcould have their worker and completion callbacks run after the hostLedgerwas destroyed during shutdown (uv_stopends the main loop, then the post-loop flushing iterations run afterLedgerhas left scope inrun_main_loop). The worker dereferenceddata->ledger, a use-after-free. Per @eddyashton's analysis, the other flagged sites (timer.h,process_launcher.h,Signal,HTTPParser) are already safe via libuv proxy/close pointers orrun-scoped lifetimes.Changes (all in
src/host/ledger.h)AsyncReadStatecontrol block -shared_ptr-owned byLedgerand copied into every outstandingAsyncLedgerGet. Holds a mutex, condition variable,active_workerscount, andledger_aliveflag. It outlives theLedger, so worker callbacks can safely decide whether they may still access it during shutdown.on_ledger_get_async- registers as an active worker under the lock; ifledger_aliveis already false it skips the read entirely (logged at DEBUG). The rawLedger*is only dereferenced after observingledger_alive == trueand incrementing the count, which forces the destructor to wait. An RAII guard decrementsactive_workersand notifies waiters on every exit path from the worker.~Ledger()- setsledger_alive = falseand blocks untilactive_workers == 0, draining in-flight reads before member destruction.Ledger-independent, writing back through a capturedto_enclaveshared_ptr.The lock ordering is safe: a worker either registers before the destructor checks (destructor waits,
Ledgerstays alive through the read) or observesledger_alive == falseand never touches theLedger. TheAsyncReadStatestays alive via the worker's ownshared_ptr, so member-destruction order is irrelevant.Scope
Shutdown-only; running nodes are unaffected. A
CHANGELOG.mdentry is included.