ext_proc filter crash when sending trailers when request has no body #27430
Conversation
Signed-off-by: Yanjun Xiang <[email protected]>
|
/assign @adisuissa @yanavlasov |
|
The detail fuzz report could be found here: https://clusterfuzz.corp.google.com/testcase-detail/5547889199546368 The crash traceback decode is: [2023-05-16 15:50:05.109][2540875][info][misc] [test/test_common/test_random_generator.cc:20] TestRandomGenerator running with seed -38107400 [2023-05-16 15:50:05.171][2540875][trace][ext_proc] [source/extensions/filters/http/ext_proc/ext_proc.cc:183] decodeHeaders: end_stream = false [2023-05-16 15:50:05.172][2540875][trace][ext_proc] [source/extensions/filters/http/ext_proc/ext_proc.cc:467] decodeTrailers
|
|
The test case to reproduce the crash is: cat test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_corpus/clusterfuzz-testcase-minimized-ext_proc_unit_test_fuzz-5547889199546368.test |
| @@ -195,7 +195,9 @@ class Filter : public Logger::Loggable<Logger::Id::ext_proc>, | |||
|
|
|||
| void sendBufferedData(ProcessorState& state, ProcessorState::CallbackState new_state, | |||
| bool end_stream) { | |||
| sendBodyChunk(state, *state.bufferedData(), new_state, end_stream); | |||
| if (state.hasBufferedData()) { | |||
There was a problem hiding this comment.
I'm not an expert on ext-proc, so high-level observation.
IIUC the current fix prevents sending a body of length 0 with end_stream. If this is the desired behavior, then I think it is ok.
There was a problem hiding this comment.
Thanks for the comments!
Actually, ext_proc filter supports sending a body of length 0:
This change is causing test failures. So, we changed it into if the no buffered data, then sends an empty body.
After that, we seeing another issue with stream_ pointer as null, so the new change also fixed it. Please check the root cause comments for details.
| @@ -195,7 +195,9 @@ class Filter : public Logger::Loggable<Logger::Id::ext_proc>, | |||
|
|
|||
| void sendBufferedData(ProcessorState& state, ProcessorState::CallbackState new_state, | |||
| bool end_stream) { | |||
| sendBodyChunk(state, *state.bufferedData(), new_state, end_stream); | |||
| if (state.hasBufferedData()) { | |||
| sendBodyChunk(state, *state.bufferedData(), new_state, end_stream); | |||
There was a problem hiding this comment.
Another observation: the call in case of an empty body, will not change the state to new_state. Is this the desired outcome?
There was a problem hiding this comment.
Yeah, that's a good catch. We should send an empty body if no buffered data as in the new diffs.
|
@stevenzzzz as an additional pair of eyes |
Signed-off-by: Yanjun Xiang <[email protected]>
|
This fuzzer test cases exposed two issues in the ext_proc filter. If the filter processing mode is set into request header: SKIP and request body :BUFFERED, at same time, when the request only contains headers and trailers then it hit two distinct crash:
|
|
/assign @htuch |
Signed-off-by: Yanjun Xiang <[email protected]>
Done! |
…nvoyproxy#27430) * ext_proc filter crash when sending trailers without sending body. Signed-off-by: Yanjun Xiang <[email protected]> Signed-off-by: Ryan Eskin <[email protected]>
ext_proc filter crash when sending trailers when request has no body.
The code is assuming the request always has body available and stored in the ext_proc buffer state when processing trailers, which leads into the crash.
Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]