golang: avoid accessing deleted decoder_callbacks (#37405)

During Golang GC there will be envoyGoFilterHttpFinalize -> deferredDeleteRequest -> getDispatcher. Before this fix, the getDispatcher will use the occasional deleted decoder callback, which will cause a crash. After this fix, we won't touch the deleted callback anymore. Risk Level: Low Testing: after 3 hours load testing, the Envoy with this fix is still running well. Signed-off-by: spacewander <[email protected]>
envoyproxy · Dec 2, 2024 · d33332a · d33332a
1 parent 683c61f
commit d33332a
Show file tree

Hide file tree

Showing 2 changed files with 8 additions and 1 deletion.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -103,6 +103,9 @@ bug_fixes:
     Fixed metric for ADS disconnection counters using Google GRPC client. This extracts the GRPC client prefix specified
     in the :ref:`google_grpc <envoy_v3_api_field_config.core.v3.GrpcService.google_grpc>` resource used for ADS, and adds
     that as a tag ``envoy_google_grpc_client_prefix`` to the Prometheus stats.
+- area: golang
+  change:
+    Fixes a crash during Golang GC caused by accessing deleted decoder_callbacks. The bug was introduced in 1.31.0.
 - area: access_log
   change: |
     Relaxed the restriction on SNI logging to allow the ``_`` character, even if

diff --git a/contrib/golang/filters/http/source/golang_filter.h b/contrib/golang/filters/http/source/golang_filter.h
@@ -234,6 +234,8 @@ class Filter : public Http::StreamFilter,
   Http::FilterTrailersStatus decodeTrailers(Http::RequestTrailerMap&) override;
   void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override {
     decoding_state_.setDecoderFilterCallbacks(callbacks);
+    // We initilizes dispatcher as soon as it is available.
+    dispatcher_ = &callbacks.dispatcher();
   }
 
   // Http::StreamEncoderFilter
@@ -307,7 +309,7 @@ class Filter : public Http::StreamFilter,
   const StreamInfo::StreamInfo& streamInfo() const { return decoding_state_.streamInfo(); }
   StreamInfo::StreamInfo& streamInfo() { return decoding_state_.streamInfo(); }
   bool isThreadSafe() { return decoding_state_.isThreadSafe(); };
-  Event::Dispatcher& getDispatcher() { return decoding_state_.getDispatcher(); }
+  Event::Dispatcher& getDispatcher() { return *dispatcher_; }
 
   bool doHeaders(ProcessorState& state, Http::RequestOrResponseHeaderMap& headers, bool end_stream);
   GolangStatus doHeadersGo(ProcessorState& state, Http::RequestOrResponseHeaderMap& headers,
@@ -358,6 +360,8 @@ class Filter : public Http::StreamFilter,
   DecodingProcessorState& decoding_state_;
   EncodingProcessorState& encoding_state_;
 
+  Event::Dispatcher* dispatcher_;
+
   // lock for has_destroyed_/etc, to avoid race between envoy c thread and go thread (when calling
   // back from go).
   Thread::MutexBasicLockable mutex_{};