wasm: fix wasm vm crash before the body too large #37079
Conversation
Signed-off-by: wangbaiping/wbpcode <wangbaiping@bytedance.com>
wbpcode
force-pushed
the
dev-fix-wasm-vm-crash
branch
from
a9257e1
to
3f83a18
Compare
|
/retest |
| @@ -1654,6 +1654,14 @@ Http::FilterDataStatus Context::decodeData(::Envoy::Buffer::Instance& data, bool | |||
| } | |||
| if (buffering_request_body_) { | |||
| decoder_callbacks_->addDecodedData(data, false); | |||
There was a problem hiding this comment.
I didn't review the previous PR, but I'm genuinely confused why this isn't causing doubling the data somehow.
There was a problem hiding this comment.
the add* actually use move semantics
There was a problem hiding this comment.
Probably the request_body_buffer_ = &data; should be in the else, it is not used if buffering. and the if should check the callback pointer is not null (probably never happends to be null) but to be consistent with the place it is used getBuffer:
| @@ -234,6 +237,49 @@ TEST_P(WasmFilterIntegrationTest, BodyBufferedMultipleChunksManipulation) { | |||
| "upstream_very_long_body.end"); | |||
| } | |||
|
|
|||
| TEST_P(WasmFilterIntegrationTest, LargeRequestHitBufferLimit) { | |||
There was a problem hiding this comment.
I have tried to make the test failed, but with and without the change, both pass.
There was a problem hiding this comment.
this test mainly used to ensure big body could works as expected and this patch doesn't break anything.
It's hard to reproduce the bug in integration test because it requires specific language sdk implementation and the data should be handled by chunks.
I think maybe I should add a unit test for this. If I can get more time.
There was a problem hiding this comment.
It cannot be reproduced by sending large body requests, as it will go through prepareLocalReplyViaFilterChain and will not immediately end the request
|
/wait-any |
Commit Message: wasm: fix wasm vm crash before the body too large
Additional Description:
To fix #36989
Risk Level: low.
Testing: unit/integration.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.