Avoid send empty body to ext_proc server if decodeData() never called by yanjunxiang-google · Pull Request #28672 · envoyproxy/envoy

yanjunxiang-google · 2023-07-27T23:38:49Z

If decodeData() is not called, currently ext_proc filter is still possible to send an empty body to ext_proc server with

envoy/source/extensions/filters/http/ext_proc/ext_proc.cc

Line 480 in b8853df

    
           sendBufferedData(state, ProcessorState::CallbackState::BufferedBodyCallback, true);

and

envoy/source/extensions/filters/http/ext_proc/ext_proc.cc

Line 565 in b8853df

sendBodyChunk(state, data, new_state, end_stream);

After body mutation from ext_proc server is received by ext_proc filter, it will modify the decoding buffer, which leads to an ASSERT() in filter manager
common/http/filter_manager.cc:426] assert failure: parent_.state_.latest_data_decoding_filter_ == this

The condition to trigger this ASSERT is ext_proc filter is configured to send body, but client only sends headers and trailers.

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:]

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only · 2023-07-27T23:38:58Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #28672 was opened by yanjunxiang-google.

see: more, trace.

yanjunxiang-google · 2023-07-27T23:46:38Z

The solution is to add a bufferedData() check before sending body to ext_proc server. bufferedData() will only become valid pointer if decodeData() is every called.

If no data received, and decodeData() is never called, it will be nullptr.

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2023-07-28T12:24:18Z

/retest

yanjunxiang-google · 2023-07-28T13:40:29Z

/assign @tyxia @yanavlasov

…_body_add Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2023-07-29T13:12:14Z

/assign @stevenzzzz

yanjunxiang-google · 2023-07-29T13:16:45Z

  }

-  if (!body_delivered && state.bodyMode() == ProcessingMode::BUFFERED) {
+  if (!body_delivered && state.bufferedData() && state.bodyMode() == ProcessingMode::BUFFERED) {


The other utility function hasBufferedData() can not be used here since it not only check bufferedData() not nullptr, also checks the data->length >0. Using hasBufferedData() here will break the decodeData() with an empty chunk case.

QQ: I am wondering when do we need to decodeData() with an empty chunk case? What are the use cases for this?

bufferedData()!=nullptr

yanjunxiang-google · 2023-07-29T13:28:30Z

This is the crash log and crash traceback decode:

test_ext_proc copy.odt

yanjunxiang-google · 2023-07-29T13:37:11Z

The root cause of the issue is this:

If ext_proc filter is configured to send body, and if only headers and trailers are received, no body is received by the ext_proc filter, it will send an empty boy to the ext_proc server.
The ext_proc server will mutate the body, and send back a body response. The body response will modify the decoding buffer, which trigger the ASSERT there:

envoy/source/common/http/filter_manager.cc

Line 426 in 522f991

ASSERT(parent_.state_.latest_data_decoding_filter_ == this);

So, filter_manager carefully maintain state_.latest_data_decoding_filter_ during decodeData():

envoy/source/common/http/filter_manager.cc

Line 658 in 522f991

    
           recordLatestDataFilter(entry, state_.latest_data_decoding_filter_, decoder_filters_);

In this case, decodeData() is never called due to client is not sending data, and data is constructed by ext_proc filter itself. That leads to latest_data_decoding_filter_ is an empty pointer not pointing the ext_proc filter

Thus the ASSERT failed, and crashed there.

Overall, the ext_proc filter should not send data based on the ext_proc filter configuration. It should only send data if the data presents. Concept wise, this issue is similar to sending trailer when trailer is not present case : #28592

yanjunxiang-google · 2023-07-29T13:49:45Z

/assign @mattklein123

yanjunxiang-google · 2023-08-01T14:11:37Z

Kind Ping

tyxia

Nice work!

tyxia · 2023-08-01T15:00:21Z

  }

-  if (!body_delivered && state.bodyMode() == ProcessingMode::BUFFERED) {
+  if (!body_delivered && state.bufferedData() && state.bodyMode() == ProcessingMode::BUFFERED) {


QQ: I am wondering when do we need to decodeData() with an empty chunk case? What are the use cases for this?

tyxia

LGTM, Thanks!

stevenzzzz

The "skipping body" approach in this PR feels like a behavior change that we can avoid by calling de/encodeData(dataMutations, false); on receiving the data mutation response from ext_server.

But no strong feelings.

stevenzzzz · 2023-08-01T18:46:25Z

  }

-  if (!body_delivered && state.bodyMode() == ProcessingMode::BUFFERED) {
+  if (!body_delivered && state.bufferedData() && state.bodyMode() == ProcessingMode::BUFFERED) {


bufferedData()!=nullptr

yanjunxiang-google · 2023-08-01T20:15:58Z

The "skipping body" approach in this PR feels like a behavior change that we can avoid by calling de/encodeData(dataMutations, false); on receiving the data mutation response from ext_server.

But no strong feelings.

Thanks for the comments!

The ext_proc behavior is now changed into "if configured as such, deliver body/trailer to the ext_proc server if they are present".

yanjunxiang-google · 2023-08-02T12:35:24Z

@envoyproxy/api-shepherds PTAL

yanavlasov · 2023-08-03T13:30:08Z

API change is comment only.

Avoid send empty body to ext_proc server if decodeData() not called

eed0899

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google requested a review from mattklein123 as a code owner July 27, 2023 23:38

repokitteh-read-only Bot added the api label Jul 27, 2023

repokitteh-read-only Bot assigned mattklein123 Jul 27, 2023

yanjunxiang-google marked this pull request as draft July 27, 2023 23:45

yanjunxiang-google added 2 commits July 27, 2023 23:53

fixing test

cdd12f3

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

adding a body send test

63619af

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google marked this pull request as ready for review July 28, 2023 13:40

Merge branch 'main' of https://github.com/envoyproxy/envoy into empty…

f68d230

…_body_add Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

mattklein123 assigned yanavlasov and tyxia and unassigned mattklein123 Jul 28, 2023

repokitteh-read-only Bot assigned stevenzzzz Jul 29, 2023

yanjunxiang-google changed the title ~~Avoid send empty body to ext_proc server if decodeData() not called~~ Avoid send empty body to ext_proc server if decodeData() never called Jul 29, 2023

yanjunxiang-google commented Jul 29, 2023

View reviewed changes

repokitteh-read-only Bot assigned mattklein123 Jul 29, 2023

mattklein123 removed their assignment Jul 30, 2023

yanavlasov approved these changes Aug 1, 2023

View reviewed changes

tyxia reviewed Aug 1, 2023

View reviewed changes

tyxia approved these changes Aug 1, 2023

View reviewed changes

stevenzzzz approved these changes Aug 1, 2023

View reviewed changes

yanavlasov merged commit 5e4f350 into envoyproxy:main Aug 3, 2023

tyxia mentioned this pull request Jan 25, 2024

ext_proc: Update the proto comment #32031

Merged

Conversation

yanjunxiang-google commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Jul 27, 2023

Uh oh!

yanjunxiang-google commented Jul 27, 2023

Uh oh!

yanjunxiang-google commented Jul 28, 2023

Uh oh!

yanjunxiang-google commented Jul 28, 2023

Uh oh!

yanjunxiang-google commented Jul 29, 2023

Uh oh!

yanjunxiang-google Jul 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyxia Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenzzzz Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google commented Jul 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanjunxiang-google commented Jul 29, 2023

Uh oh!

yanjunxiang-google commented Jul 29, 2023

Uh oh!

yanjunxiang-google commented Aug 1, 2023

Uh oh!

tyxia left a comment

Choose a reason for hiding this comment

Uh oh!

tyxia Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tyxia left a comment

Choose a reason for hiding this comment

Uh oh!

stevenzzzz left a comment

Choose a reason for hiding this comment

Uh oh!

stevenzzzz Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yanjunxiang-google commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanjunxiang-google commented Aug 2, 2023

Uh oh!

yanavlasov commented Aug 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yanjunxiang-google commented Jul 27, 2023 •

edited

Loading

yanjunxiang-google Jul 29, 2023 •

edited

Loading

tyxia Aug 1, 2023 •

edited

Loading

yanjunxiang-google commented Jul 29, 2023 •

edited

Loading

tyxia Aug 1, 2023 •

edited

Loading

yanjunxiang-google commented Aug 1, 2023 •

edited

Loading