`addAfterVmCallAction` may cause unexpected problems

[johnlanni]

addAfterVmCallAction and doAfterVmCallActions are used by the context to execute functions after vm calls, such as onRequestHeaders:

https://github.com/proxy-wasm/proxy-wasm-cpp-host/blob/72ce32f7b11f9190edf874028255e1309e41690f/src/context.cc#L312-L324

If sendLocalReply has been added to after_vm_call_actions_ via addAfterVmCallAction:

https://github.com/envoyproxy/envoy/blob/main/source/extensions/common/wasm/context.cc#L1635-L1645

Here f() will execute sendLocalReply:

https://github.com/proxy-wasm/proxy-wasm-cpp-host/blob/72ce32f7b11f9190edf874028255e1309e41690f/include/proxy-wasm/wasm.h#L154

It will call encodeHeaders() which triggers the vm to call onResponseHeaders as follows:

https://github.com/proxy-wasm/proxy-wasm-cpp-host/blob/72ce32f7b11f9190edf874028255e1309e41690f/src/context.cc#L374-L384

Then call doAfterVmCallActions again, that is, the function is re-entered, and the functions in after_vm_call_actions_ left in the onRequestHeaders stage will be executed in the onResonseHeaders stage, which may cause unexpected problems.

The following code compiled to wasm crashes Envoy:

func (ctx *httpHeaders) OnHttpRequestHeaders(numHeaders int, endOfStream bool) types.Action {
      proxywasm.DispatchHttpCall("xxxxx", headers, nil, nil, 500, func(numHeaders, bodySize, numTrailers int) {
            proxywasm.SendHttpResponse(200, nil, nil, -1)
            proxywasm.ResumeHttpRequest()
      })
     return types.ActionPause
}

ResumeHttpRequest will call continueStream and add the decoder_callbacks_->continueDecoding() function to after_vm_call_actions_, which will be executed during the onResonseHeaders stage triggered by sendLocalReply.

https://github.com/envoyproxy/envoy/blob/a77335a730f567502721319b20c4b0fab10e09bc/source/extensions/common/wasm/context.cc#L1497-L1501

This resolved issue was also caused by this mechanism.

[johnlanni]

cc @PiotrSikora

[PiotrSikora]

@johnlanni thank you for digging into it!

I agree that under no circumstances Wasm should be able to crash the host, so this is definitely a serious issues, but I don't think it's caused by addAfterVmCallAction itself.

There are at least two issues here:

1. I think SendHttpResponse should be a "final" call, so no Wasm code after it should be executed, definitely not ResumeHttpRequest. https://github.com/envoyproxy/envoy/commit/ff49762696b2e6ed3f408a22e1f7a1b7d2487318 fixed this for double SendHttpResponse, but I think it was a very localized fix.
2. Calling onResponse{Headers,Body,Trailers} callbacks for a response generated in the same Proxy-Wasm plugin via SendHttpResponse doesn't make any sense. This is how Envoy works, but it's been annoying me for a while, and we should probably add a guard that prevents this from happening.

Unfortunately, both of those are breaking changes (although, the first one shouldn't be noticeable), but we should address them sooner than later.

cc @mpwarres

[johnlanni]

@PiotrSikora Maybe we should add the stage of the function in addAfterVmCallAction and check whether the stage of executing the function is consistent with the current stage in doAfterVmCallActions, so that the function escape can be avoided.