lua filter: ensure active_request_ is verified before deletion Fixes envoyproxy#38017 (envoyproxy#38863)

Lua filter was removing ":status" and then setting a response body,
which was causing envoy to terminate with a SIGSEGV. The issue occurred
because "onEncodeComplete()" attempted to delete "activerequest" via
"deferredDelete()", but "activerequest" had already been deleted
earlier, leading to a segmentation fault. This fix ensures that
"activerequest" is only deleted once, preventing double deletion. This
is done by verifying that "activerequest" is not null before the
existing check of "activerequest->remotecomplete".


Risk Level: Low

Testing:
I added a new integration test in "lua_integration_test.cc" file
specifically for this bug. Without the fix, the test fails; with it
applied, the test passes successfully.

Fixes envoyproxy#38017

---------

Signed-off-by: Leonor Figueira <[email protected]>

Author: leocfig

source/common/http/filter_manager.cc

@@ -84,6 +84,12 @@ void ActiveStreamFilterBase::commonContinue() {
     }
   }
 
+  if (!canContinue()) {
+    ENVOY_STREAM_LOG(trace, "cannot continue filter chain: filter={}", *this,
+                     static_cast<const void*>(this));
+    return;
+  }
+
   // Make sure that we handle the zero byte data frame case. We make no effort to optimize this
   // case in terms of merging it into a header only request/response. This could be done in the
   // future.
@@ -92,8 +98,20 @@ void ActiveStreamFilterBase::commonContinue() {
     doHeaders(observedEndStream() && !bufferedData() && !hasTrailers());
   }
 
+  if (!canContinue()) {
+    ENVOY_STREAM_LOG(trace, "cannot continue filter chain: filter={}", *this,
+                     static_cast<const void*>(this));
+    return;
+  }
+
   doMetadata();
 
+  if (!canContinue()) {
+    ENVOY_STREAM_LOG(trace, "cannot continue filter chain: filter={}", *this,
+                     static_cast<const void*>(this));
+    return;
+  }
+
   // It is possible for trailers to be added during doData(). doData() itself handles continuation
   // of trailers for the non-continuation case. Thus, we must keep track of whether we had
   // trailers prior to calling doData(). If we do, then we continue them here, otherwise we rely
@@ -103,6 +121,12 @@ void ActiveStreamFilterBase::commonContinue() {
     doData(observedEndStream() && !had_trailers_before_data);
   }
 
+  if (!canContinue()) {
+    ENVOY_STREAM_LOG(trace, "cannot continue filter chain: filter={}", *this,
+                     static_cast<const void*>(this));
+    return;
+  }
+
   if (had_trailers_before_data) {
     doTrailers();
   }

test/extensions/filters/http/lua/lua_integration_test.cc

@@ -2026,5 +2026,66 @@ name: lua
   cleanup();
 }
 
+#ifdef NDEBUG
+// This test is only run in release mode because in debug mode,
+// the code reaches ENVOY_BUG() which triggers a forced abort
+// that stops execution.
+TEST_P(LuaIntegrationTest, ModifyResponseBodyAndRemoveStatusHeader) {
+  if (downstream_protocol_ != Http::CodecType::HTTP1) {
+    GTEST_SKIP() << "This is a test that only supports http1";
+  }
+  if (!testing_downstream_filter_) {
+    GTEST_SKIP() << "This is a local reply test that does not go upstream";
+  }
+  const std::string filter_config =
+      R"EOF(
+name: lua
+typed_config:
+  "@type": type.googleapis.com/envoy.extensions.filters.http.lua.v3.Lua
+  default_source_code:
+    inline_string: |
+      function envoy_on_response(response_handle)
+        local response_headers = response_handle:headers()
+        response_headers:remove(":status")
+        response_handle:body(true):setBytes("hello world")
+      end
+)EOF";
+
+  const std::string route_config =
+      R"EOF(
+name: basic_lua_routes
+virtual_hosts:
+- name: rds_vhost_1
+  domains: ["lua.per.route"]
+  routes:
+  - match:
+      prefix: "/lua"
+    direct_response:
+      status: 200
+      body:
+        inline_string: "hello"
+)EOF";
+
+  initializeWithYaml(filter_config, route_config);
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+
+  Http::TestRequestHeaderMapImpl default_headers{{":method", "GET"},
+                                                 {":path", "/lua"},
+                                                 {":scheme", "http"},
+                                                 {":authority", "lua.per.route"},
+                                                 {"x-forwarded-for", "10.0.0.1"}};
+
+  auto encoder_decoder = codec_client_->startRequest(default_headers);
+  auto response = std::move(encoder_decoder.second);
+
+  ASSERT_TRUE(response->waitForEndStream());
+  ASSERT_TRUE(response->complete());
+  EXPECT_EQ("502", response->headers().getStatusValue());
+  EXPECT_THAT(response->body(), testing::HasSubstr("missing required header: :status"));
+
+  cleanup();
+}
+#endif
+
 } // namespace
 } // namespace Envoy