hcm: Ensure operations are not called on deleted stream decoders #39346
Conversation
Previously, it was possible for the `ActiveStream` in the `HttpConnectionManager` to get deleted while still trying to process packets from the codec. This change uses the `ActiveStreamHandle` with weak pointer semantics to ensure that even with incoming data packets, methods are not called on a deleted `ActiveStream`, which represents the HCM's `RequestDecoder`. Signed-off-by: Ali Beyad <[email protected]>
|
cc @wu-bin |
|
/wait |
There was a problem hiding this comment.
Thank you for working on this.
IIUC we use the ActiveStreamHandle weak pointer semantics (the shared ptr gets dtored on when the stream is deleted) to check that the HCM ActiveStream is still alive. We only need to do this for server-side streams since it's only an issue with HCM interactions between the two.
A release note in current.yaml is probably worth adding as a bug fix -- and CI is failing.
Otherwise LGTM.
| @@ -608,8 +617,12 @@ void ConnectionImpl::ServerStreamImpl::decodeTrailers() { | |||
| // Consume any buffered trailers. | |||
| stream_manager_.trailers_buffered_ = false; | |||
|
|
|||
| request_decoder_->decodeTrailers( | |||
| std::move(absl::get<RequestTrailerMapPtr>(headers_or_trailers_))); | |||
| RequestDecoder* request_decoder = request_decoder_handle_->get().ptr(); | |||
There was a problem hiding this comment.
Can we just call decoder() method here? like RequestDecoder* request_decoder = decoder();
There was a problem hiding this comment.
Actually the reason I did that is decoder() returns a StreamDecoder* which doesn't have methods to decode headers or trailers, hence why I directly get the RequestDecoder*.
| @@ -585,7 +585,9 @@ class ConnectionImpl : public virtual Connection, | |||
| // written out before force resetting the stream, assuming there is enough H2 connection flow | |||
| // control window is available. | |||
| bool useDeferredReset() const override { return true; } | |||
| StreamDecoder& decoder() override { return *request_decoder_; } | |||
| StreamDecoder* decoder() override { | |||
| return request_decoder_handle_->get().ptr(); | |||
There was a problem hiding this comment.
get() should get you the raw pointer. Is the .ptr() redundant?
There was a problem hiding this comment.
get() gets an OptRef:
envoy/source/common/http/conn_manager_impl.h
Line 533 in 1e68b78
ptr() to get the raw pointer.
Signed-off-by: Ali Beyad <[email protected]>
Thanks, added a note to current.yaml. |
Signed-off-by: Ali Beyad <[email protected]>
Signed-off-by: Ali Beyad <[email protected]>
Signed-off-by: Ali Beyad <[email protected]>
Previously, it was possible for the
ActiveStreamin theHttpConnectionManagerto get deleted while still trying to process packets from the codec.This change uses the
ActiveStreamHandlewith weak pointer semantics to ensure that even with incoming data packets, methods are not called on a deletedActiveStream, which represents the HCM'sRequestDecoder.