From d33332a12e46da3049a8ed02571a6764e7f21024 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BD=97=E6=B3=BD=E8=BD=A9?= Date: Tue, 3 Dec 2024 03:13:28 +0800 Subject: [PATCH] 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 --- changelogs/current.yaml | 3 +++ contrib/golang/filters/http/source/golang_filter.h | 6 +++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 4aae63c7a7c6..afbb674e9363 100644 --- 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 ` 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 index 183d4635ada5..4a68b250bac2 100644 --- 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_{};