Skip to content

fix: make WasmBase::doAfterVmCallActions reentry-safe (drain-to-local)#558

Open
johnlanni wants to merge 1 commit into
proxy-wasm:mainfrom
higress-group:fix/doAfterVmCallActions-reentry-safe
Open

fix: make WasmBase::doAfterVmCallActions reentry-safe (drain-to-local)#558
johnlanni wants to merge 1 commit into
proxy-wasm:mainfrom
higress-group:fix/doAfterVmCallActions-reentry-safe

Conversation

@johnlanni

Copy link
Copy Markdown
Contributor

Problem

WasmBase::doAfterVmCallActions() drains its after_vm_call_actions_ queue with an
unguarded while (!after_vm_call_actions_.empty()) loop, popping and running one action
at a time:

while (!self->after_vm_call_actions_.empty()) {
  auto f = std::move(self->after_vm_call_actions_.front());
  self->after_vm_call_actions_.pop_front();
  f();
}

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 this loop never sees an empty queue and spins forever,
pinning a worker at 100% CPU.

This is reachable from a plugin today. A host call made from within an after-VM-call
action re-enters the VM synchronously, and that nested VM call's DeferAfterCallActions
guard calls doAfterVmCallActions() again. For example, in the Envoy host, a deferred
onRedisCall/onHttpCall failure callback that calls
injectEncodedDataToFilterChain (or sendLocalReply) re-enters the VM and schedules
another after-VM-call action; the outer loop keeps observing a non-empty queue and never
returns.

The same shared-queue reentry is also the mechanism behind #326: an action queued in one
phase (e.g. continueStream() from onRequestHeaders) is drained by a nested
doAfterVmCallActions() belonging to a different phase (e.g. onResponseHeaders
triggered by sendLocalReply), so the action "escapes" into the wrong phase.

Fix

Swap the member queue into a local std::deque before draining, then iterate the
snapshot exactly once:

std::deque<std::function<void()>> local;
local.swap(self->after_vm_call_actions_);
for (auto &f : local) {
  f();
}

Actions re-added by a callback during the drain land in the (now-empty) member queue and
are therefore picked up by the next-outer DeferAfterCallActions frame, instead of being
re-run in this loop. This:

  • Breaks the synchronous-reentry spin (bounded work per call).
  • Prevents cross-phase action escape (the re-added action runs in the outer frame, not
    inside a nested VM call of another phase) — see addAfterVmCallAction may cause unexpected problems #326.
  • Preserves ordering semantics for legitimately deferred work; re-added actions are still
    executed, just deferred to the next-outer frame.

It is a header-only change to a small inline method, zero-ABI and transparent to plugins
(no signature or behavior change visible to Wasm modules).

Test

Adds TEST_P(TestVm, DoAfterVmCallActionsReentrySafe) in test/wasm_test.cc, wired into
the existing //test:wasm_test target. It registers a self-re-enqueuing action and
asserts the count advances by exactly one per doAfterVmCallActions() call. On the old
code this test hangs forever (documenting the regression); with the fix each drain runs
the snapshot once and defers the re-added copy.

Refs

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 proxy-wasm#326).

Adds a regression test (DoAfterVmCallActionsReentrySafe) that hangs
forever on the old code and passes with the fix.

Signed-off-by: 澄潭 <zty98751@alibaba-inc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant