From f1f246e37bd4a06f24b081a3422c26783087ffed Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Thu, 25 May 2023 18:55:50 +0000 Subject: [PATCH 1/5] ext_proc fuzzer test exposed a race issue which leads to ENVOY_BUG in ext_proc filter when opens a gRPC stream. Signed-off-by: Yanjun Xiang --- .../filters/http/ext_proc/ext_proc.cc | 11 +-- ...h-232906f11069ff930e29c13c1a3ae5a2c3202c57 | 70 +++++++++++++++++++ 2 files changed, 76 insertions(+), 5 deletions(-) create mode 100644 test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_corpus/crash-232906f11069ff930e29c13c1a3ae5a2c3202c57 diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index deb93c625667..df0eae2563e1 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -118,15 +118,16 @@ void Filter::setEncoderFilterCallbacks(Http::StreamEncoderFilterCallbacks& callb } Filter::StreamOpenState Filter::openStream() { - ENVOY_BUG(!processing_complete_, "openStream should not have been called"); + // If processing already completes, no need to open a gRPC stream. + if (processing_complete_) { + // Stream failed while starting and either onGrpcError or onGrpcClose was already called + return sent_immediate_response_ ? StreamOpenState::Error : StreamOpenState::IgnoreError; + } if (!stream_) { ENVOY_LOG(debug, "Opening gRPC stream to external processor"); stream_ = client_->start(*this, grpc_service_, decoder_callbacks_->streamInfo()); stats_.streams_started_.inc(); - if (processing_complete_) { - // Stream failed while starting and either onGrpcError or onGrpcClose was already called - return sent_immediate_response_ ? StreamOpenState::Error : StreamOpenState::IgnoreError; - } + // For custom access logging purposes. Applicable only for Envoy gRPC as Google gRPC does not // have a proper implementation of streamInfo. if (grpc_service_.has_envoy_grpc()) { diff --git a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_corpus/crash-232906f11069ff930e29c13c1a3ae5a2c3202c57 b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_corpus/crash-232906f11069ff930e29c13c1a3ae5a2c3202c57 new file mode 100644 index 000000000000..9bfa96c62f75 --- /dev/null +++ b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_corpus/crash-232906f11069ff930e29c13c1a3ae5a2c3202c57 @@ -0,0 +1,70 @@ +config { + grpc_service { + google_grpc { + target_uri: "test_uri" + channel_credentials { + ssl_credentials { + private_key { + inline_string: "/" + } + } + } + stat_prefix: "test_uri" + credentials_factory_name: "test_uri" + config { + } + } + initial_metadata { + key: "test_uri" + } + } + failure_mode_allow: true + processing_mode { + response_header_mode: SKIP + } + request_attributes: "(" + response_attributes: "test_uri" + response_attributes: "9\000\000\000\000\000\000\000" + stat_prefix: "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000." +} +request { + headers { + headers { + key: "(" + } + headers { + key: "test_uri" + } + } + http_body { + data: "\360\270\270\270\360\270\270\270\360\270\270\270\360\270\270\270\360\270\270\270\360\270\270\270\360\270\270\270\360\270\270\270\360\270\270\270\360\270\270\270\360\270\270\270\360\270\270\270\350\270\270" + } +} +response { + request_headers { + response { + header_mutation { + remove_headers: "f" + remove_headers: "f" + } + } + } + dynamic_metadata { + fields { + key: "" + value { + } + } + fields { + key: "" + value { + } + } + } + mode_override { + request_header_mode: SKIP + request_body_mode: BUFFERED + response_body_mode: BUFFERED + request_trailer_mode: SEND + } +} From 3bfe6447723f4c702e1d51b1c0c957666ab3265e Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 31 May 2023 03:45:13 +0000 Subject: [PATCH 2/5] adding stream_ check Signed-off-by: Yanjun Xiang --- source/extensions/filters/http/ext_proc/ext_proc.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 37b5fd8ebc09..a631b702a672 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -120,14 +120,16 @@ void Filter::setEncoderFilterCallbacks(Http::StreamEncoderFilterCallbacks& callb Filter::StreamOpenState Filter::openStream() { // If processing already completes, no need to open a gRPC stream. if (processing_complete_) { - // Stream failed while starting and either onGrpcError or onGrpcClose was already called return sent_immediate_response_ ? StreamOpenState::Error : StreamOpenState::IgnoreError; } if (!stream_) { ENVOY_LOG(debug, "Opening gRPC stream to external processor"); stream_ = client_->start(*this, grpc_service_, decoder_callbacks_->streamInfo()); - stats_.streams_started_.inc(); + if (stream_ == nullptr) { + return config_->failureModeAllow() ? StreamOpenState::IgnoreError : StreamOpenState::Error; + } + stats_.streams_started_.inc(); // For custom access logging purposes. Applicable only for Envoy gRPC as Google gRPC does not // have a proper implementation of streamInfo. if (grpc_service_.has_envoy_grpc()) { From 7e3f10bffb6e13cbb421c2b5cefbe5e55c89d231 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Wed, 31 May 2023 17:37:46 +0000 Subject: [PATCH 3/5] addressing comments Signed-off-by: Yanjun Xiang --- source/extensions/filters/http/ext_proc/ext_proc.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index a631b702a672..d79679d17c3b 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -118,18 +118,18 @@ void Filter::setEncoderFilterCallbacks(Http::StreamEncoderFilterCallbacks& callb } Filter::StreamOpenState Filter::openStream() { - // If processing already completes, no need to open a gRPC stream. + // External processing already completes. Return IgnoreError so the filter will return Continue. if (processing_complete_) { - return sent_immediate_response_ ? StreamOpenState::Error : StreamOpenState::IgnoreError; + return StreamOpenState::IgnoreError; } if (!stream_) { ENVOY_LOG(debug, "Opening gRPC stream to external processor"); stream_ = client_->start(*this, grpc_service_, decoder_callbacks_->streamInfo()); - if (stream_ == nullptr) { - return config_->failureModeAllow() ? StreamOpenState::IgnoreError : StreamOpenState::Error; - } - stats_.streams_started_.inc(); + if (processing_complete_) { + // Stream failed while starting and either onGrpcError or onGrpcClose was already called + return sent_immediate_response_ ? StreamOpenState::Error : StreamOpenState::IgnoreError; + } // For custom access logging purposes. Applicable only for Envoy gRPC as Google gRPC does not // have a proper implementation of streamInfo. if (grpc_service_.has_envoy_grpc()) { From 595f03817a75f90ce38e6532219a5b8017765ffa Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Fri, 2 Jun 2023 17:29:51 +0000 Subject: [PATCH 4/5] ENVOY_LOG Signed-off-by: Yanjun Xiang --- source/extensions/filters/http/ext_proc/ext_proc.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index d79679d17c3b..714548f3ba22 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -120,6 +120,7 @@ void Filter::setEncoderFilterCallbacks(Http::StreamEncoderFilterCallbacks& callb Filter::StreamOpenState Filter::openStream() { // External processing already completes. Return IgnoreError so the filter will return Continue. if (processing_complete_) { + ENVOY_LOG(debug, "External processing already completed when trying to open the gRPC stream"); return StreamOpenState::IgnoreError; } if (!stream_) { From 4bea75d33e89a9976bd1343af86677977e770898 Mon Sep 17 00:00:00 2001 From: Yanjun Xiang Date: Fri, 2 Jun 2023 17:53:42 +0000 Subject: [PATCH 5/5] addressing comments Signed-off-by: Yanjun Xiang --- source/extensions/filters/http/ext_proc/ext_proc.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index 714548f3ba22..0ef4f84c8274 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -118,9 +118,11 @@ void Filter::setEncoderFilterCallbacks(Http::StreamEncoderFilterCallbacks& callb } Filter::StreamOpenState Filter::openStream() { - // External processing already completes. Return IgnoreError so the filter will return Continue. + // External processing is completed. This means there is no need to send any further + // message to the server for processing. Just return IgnoreError so the filter + // will return FilterHeadersStatus::Continue. if (processing_complete_) { - ENVOY_LOG(debug, "External processing already completed when trying to open the gRPC stream"); + ENVOY_LOG(debug, "External processing is completed when trying to open the gRPC stream"); return StreamOpenState::IgnoreError; } if (!stream_) {