| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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>
|
Thank you for sending this fix! It looks good overall, but I'm not sure it's safe to defer actions past the frame where it is registered. Why don't we want to execute that action in the same drain where it is registered? I anticipate this would be more correct: while (!after_vm_call_actions_.empty()) {
std::deque<std::function<void()>> local;
local.swap(after_vm_call_actions_);
for (auto& f : local) {
f();
}
}
This guarantees that:
|
Sorry, something went wrong.
|
(CC @PiotrSikora for comment, as iiuc the original author of the Envoy bits using AfterVmCallActions) |
Sorry, something went wrong.
|
Thanks for working on this, @johnlanni! As I've mentioned 3 years ago in #326 (comment), I think the root cause of many of those problems is that Envoy calls on_response_* callbacks for the response generated from within the Wasm plugin, which is very weird behavior... I believe that someone was supposed to fix/change that, but it doesn't look like it happened. As for this change - I didn't touch this codebase in many years, but if I understand it correctly, then the deferred actions added in doAfterVmCallActions() at the end of the current callback would be delayed until doAfterVmCallActions() executed at the end of the next callback... However, there is no guarantee that the next callback would be executed in the context of the same request, so this would leak information across different requests, and lead to all kinds of trouble. |
Sorry, something went wrong.
@leonm1 I don't think this changes anything vs existing code, does it? |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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:
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:
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:
inside a nested VM call of another phase) — see addAfterVmCallAction may cause unexpected problems #326.
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
deferred Redis-failure callback that injects into the filter chain).