From 4f9961facfd656c277cf4c8ad3a905922cce25fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=BE=84=E6=BD=AD?= Date: Tue, 7 Jul 2026 16:11:48 +0800 Subject: [PATCH] fix: make WasmBase::doAfterVmCallActions reentry-safe (drain-to-local) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit doAfterVmCallActions() drained after_vm_call_actions_ with an unguarded `while (!empty())` loop, popping and running one action at a time. If an action re-enqueues itself (or otherwise adds a new action) during the drain, the freshly-added action is observed by the same loop and executed again in the same frame. Under synchronous reentry -- e.g. a host call such as sendLocalReply / injectEncodedDataToFilterChain that re-enters the VM and schedules another after-VM-call action -- the loop never sees an empty queue and spins forever, pinning a CPU at 100%. Swap the member queue into a local std::deque before draining, then iterate the snapshot exactly once. Actions re-added during the drain land in the (now-empty) member queue and are picked up by the next-outer DeferAfterCallActions frame instead of being re-run in this loop. This also prevents an action queued in one phase from executing inside a nested VM call of a different phase (see #326). Adds a regression test (DoAfterVmCallActionsReentrySafe) that hangs forever on the old code and passes with the fix. Signed-off-by: 澄潭 --- include/proxy-wasm/wasm.h | 18 +++++++++++++++--- test/wasm_test.cc | 28 ++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/include/proxy-wasm/wasm.h b/include/proxy-wasm/wasm.h index 10c16a794..74c69a6b2 100644 --- a/include/proxy-wasm/wasm.h +++ b/include/proxy-wasm/wasm.h @@ -147,9 +147,21 @@ class WasmBase : public std::enable_shared_from_this { // NB: this may be deleted by a delayed function unless prevented. if (!after_vm_call_actions_.empty()) { auto self = shared_from_this(); - while (!self->after_vm_call_actions_.empty()) { - auto f = std::move(self->after_vm_call_actions_.front()); - self->after_vm_call_actions_.pop_front(); + // Swap the queue into a local before draining, then iterate the snapshot exactly + // once. Actions re-added by a callback during the drain land in the (now-empty) + // member queue and are picked up by the next-outer DeferAfterCallActions frame, + // instead of being re-run in this same loop. With the old unguarded + // `while (!empty())`, an action that (directly or transitively) re-enqueues during + // the drain -- e.g. a host call such as sendLocalReply/injectEncodedDataToFilterChain + // that synchronously re-enters the VM and schedules another after-VM-call action -- + // means the loop never observes an empty queue and spins forever, pinning a CPU at + // 100%. Deferring re-added actions to the next-outer frame also prevents an action + // queued in one phase (e.g. continueStream from onRequestHeaders) from executing + // inside a nested VM call of a different phase (e.g. onResponseHeaders triggered by + // sendLocalReply); see proxy-wasm/proxy-wasm-cpp-host#326. + std::deque> local; + local.swap(self->after_vm_call_actions_); + for (auto &f : local) { f(); } } diff --git a/test/wasm_test.cc b/test/wasm_test.cc index 2a3e60cc4..7aed8330d 100644 --- a/test/wasm_test.cc +++ b/test/wasm_test.cc @@ -324,4 +324,32 @@ TEST_P(TestVm, CleanupThreadLocalCacheKeys) { EXPECT_TRUE(stale_wasms_keys.empty()); } +// doAfterVmCallActions() must not spin forever when a queued action re-enqueues itself +// during the drain. With the old unguarded `while (!empty())` loop this loops forever; +// with the drain-to-local fix each call runs the snapshot exactly once and the re-added +// copy is deferred to the next drain. +TEST_P(TestVm, DoAfterVmCallActionsReentrySafe) { + // WasmBase::doAfterVmCallActions() calls shared_from_this(), so the instance must be + // owned by a std::shared_ptr (a stack WasmBase would crash). + auto wasm = std::make_shared(makeVm(engine_)); + + int count = 0; + std::function f; + f = [&]() { + ++count; + // Re-enqueue self during the drain. Under the old code this would be picked up by the + // same loop and never terminate. + wasm->addAfterVmCallAction(f); + }; + + wasm->addAfterVmCallAction(f); + // Snapshot {f} runs exactly once; the re-added copy lands in the now-empty member queue. + wasm->doAfterVmCallActions(); + EXPECT_EQ(count, 1); // Returns after one pass (the old while-loop would never return). + + // The re-added copy runs on the next frame (and re-enqueues once more). + wasm->doAfterVmCallActions(); + EXPECT_EQ(count, 2); +} + } // namespace proxy_wasm