addAfterVmCallAction may cause unexpected problems

[johnlanni]

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

proxy-wasm-cpp-host/src/context.cc

Lines 312 to 324 in 72ce32f

	FilterHeadersStatus ContextBase::onRequestHeaders(uint32_t headers, bool end_of_stream) {
	CHECK_FAIL_HTTP(FilterHeadersStatus::Continue, FilterHeadersStatus::StopAllIterationAndWatermark);
	if (!wasm_->on_request_headers_abi_01_ && !wasm_->on_request_headers_abi_02_) {
	return FilterHeadersStatus::Continue;
	}
	DeferAfterCallActions actions(this);
	const auto result = wasm_->on_request_headers_abi_01_
	? wasm_->on_request_headers_abi_01_(this, id_, headers)
	: wasm_->on_request_headers_abi_02_(this, id_, headers,
	static_cast<uint32_t>(end_of_stream));
	CHECK_FAIL_HTTP(FilterHeadersStatus::Continue, FilterHeadersStatus::StopAllIterationAndWatermark);
	return convertVmCallResultToFilterHeadersStatus(result);
	}

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:

proxy-wasm-cpp-host/include/proxy-wasm/wasm.h

Line 154 in 72ce32f

f();

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

proxy-wasm-cpp-host/src/context.cc

Lines 374 to 384 in 72ce32f

	FilterDataStatus ContextBase::onResponseBody(uint32_t body_length, bool end_of_stream) {
	CHECK_FAIL_HTTP(FilterDataStatus::Continue, FilterDataStatus::StopIterationNoBuffer);
	if (!wasm_->on_response_body_) {
	return FilterDataStatus::Continue;
	}
	DeferAfterCallActions actions(this);
	const auto result =
	wasm_->on_response_body_(this, id_, body_length, static_cast<uint32_t>(end_of_stream));
	CHECK_FAIL_HTTP(FilterDataStatus::Continue, FilterDataStatus::StopIterationNoBuffer);
	return convertVmCallResultToFilterDataStatus(result);
	}

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. envoyproxy/envoy@ff49762 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.