quiche: fix stream trailer decoding issue

bazel/external/quiche.BUILD

@@ -3654,6 +3654,35 @@ envoy_cc_test_library(
     ],
 )
 
+envoy_cc_test_library(
+    name = "quic_test_tools_qpack_qpack_encoder_test_utils_lib",
+    srcs = ["quiche/quic/test_tools/qpack/qpack_encoder_test_utils.cc"],
+    hdrs = ["quiche/quic/test_tools/qpack/qpack_encoder_test_utils.h"],
+    copts = quiche_copts,
+    repository = "@envoy",
+    tags = ["nofips"],
+    deps = [
+        ":quic_core_qpack_qpack_encoder_lib",
+        ":quic_platform_test",
+        ":quic_test_tools_qpack_qpack_test_utils_lib",
+        ":spdy_core_header_block_lib",
+        ":spdy_core_hpack_hpack_lib",
+    ],
+)
+
+envoy_cc_test_library(
+    name = "quic_test_tools_qpack_qpack_test_utils_lib",
+    srcs = ["quiche/quic/test_tools/qpack/qpack_test_utils.cc"],
+    hdrs = ["quiche/quic/test_tools/qpack/qpack_test_utils.h"],
+    copts = quiche_copts,
+    repository = "@envoy",
+    tags = ["nofips"],
+    deps = [
+        ":quic_core_qpack_qpack_stream_sender_delegate_lib",
+        ":quic_platform_test",
+    ],
+)
+
 envoy_cc_test_library(
     name = "quic_test_tools_sent_packet_manager_peer_lib",
     srcs = ["quiche/quic/test_tools/quic_sent_packet_manager_peer.cc"],

source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc

@@ -72,6 +72,10 @@ Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap&
 void EnvoyQuicClientStream::encodeData(Buffer::Instance& data, bool end_stream) {
   ENVOY_STREAM_LOG(debug, "encodeData (end_stream={}) of {} bytes.", *this, end_stream,
                    data.length());
+  if (data.length() == 0 && !end_stream) {
+    return;
+  }
+  ASSERT(!local_end_stream_);
   local_end_stream_ = end_stream;
   // This is counting not serialized bytes in the send buffer.
   const uint64_t bytes_to_send_old = BufferedDataBytes();
@@ -112,8 +116,6 @@ void EnvoyQuicClientStream::encodeMetadata(const Http::MetadataMapVector& /*meta
 }
 
 void EnvoyQuicClientStream::resetStream(Http::StreamResetReason reason) {
-  // Higher layers expect calling resetStream() to immediately raise reset callbacks.
-  runResetCallbacks(reason);
   Reset(envoyResetReasonToQuicRstError(reason));
 }
 
@@ -130,13 +132,15 @@ void EnvoyQuicClientStream::switchStreamBlockState(bool should_block) {
 
 void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len,
                                                      const quic::QuicHeaderList& header_list) {
-  quic::QuicSpdyStream::OnInitialHeadersComplete(fin, frame_len, header_list);
   if (rst_sent()) {
     return;
   }
-  ASSERT(headers_decompressed());
+  quic::QuicSpdyStream::OnInitialHeadersComplete(fin, frame_len, header_list);
+  ASSERT(headers_decompressed() && !header_list.empty());
+
   response_decoder_->decodeHeaders(
-      quicHeadersToEnvoyHeaders<Http::ResponseHeaderMapImpl>(header_list), /*end_stream=*/fin);
+      quicHeadersToEnvoyHeaders<Http::ResponseHeaderMapImpl>(header_list),
+      /*end_stream=*/fin);
   if (fin) {
     end_stream_decoded_ = true;
   }
@@ -165,18 +169,17 @@ void EnvoyQuicClientStream::OnBodyAvailable() {
     buffer->commit(&slice, 1);
     MarkConsumed(bytes_read);
   }
+  ASSERT(buffer->length() == 0 || !end_stream_decoded_);
 
-  // True if no trailer and FIN read.
-  bool finished_reading = IsDoneReading();
-  bool empty_payload_with_fin = buffer->length() == 0 && fin_received();
+  bool fin_read_and_no_trailers = IsDoneReading();
   // If this call is triggered by an empty frame with FIN which is not from peer
   // but synthesized by stream itself upon receiving HEADERS with FIN or
   // TRAILERS, do not deliver end of stream here. Because either decodeHeaders
   // already delivered it or decodeTrailers will be called.
-  bool skip_decoding = empty_payload_with_fin && (end_stream_decoded_ || !finished_reading);
+  bool skip_decoding = (buffer->length() == 0 && !fin_read_and_no_trailers) || end_stream_decoded_;
   if (!skip_decoding) {
-    response_decoder_->decodeData(*buffer, finished_reading);
-    if (finished_reading) {
+    response_decoder_->decodeData(*buffer, fin_read_and_no_trailers);
+    if (fin_read_and_no_trailers) {
       end_stream_decoded_ = true;
     }
   }
@@ -191,14 +194,10 @@ void EnvoyQuicClientStream::OnBodyAvailable() {
     return;
   }
 
-  if (!quic::VersionUsesHttp3(transport_version()) && !FinishedReadingTrailers()) {
-    // For Google QUIC implementation, trailers may arrived earlier and wait to
-    // be consumed after reading all the body. Consume it here.
-    // IETF QUIC shouldn't reach here because trailers are sent on same stream.
-    response_decoder_->decodeTrailers(
-        spdyHeaderBlockToEnvoyHeaders<Http::ResponseTrailerMapImpl>(received_trailers()));
-    MarkTrailersConsumed();
-  }
+  // Trailers may arrived earlier and wait to be consumed after reading all the body. Consume it
+  // here.
+  maybeDecodeTrailers();
+
   OnFinRead();
   in_decode_data_callstack_ = false;
 }
@@ -207,20 +206,31 @@ void EnvoyQuicClientStream::OnTrailingHeadersComplete(bool fin, size_t frame_len
                                                       const quic::QuicHeaderList& header_list) {
   quic::QuicSpdyStream::OnTrailingHeadersComplete(fin, frame_len, header_list);
   ASSERT(trailers_decompressed());
-  if (session()->connection()->connected() &&
-      (quic::VersionUsesHttp3(transport_version()) || sequencer()->IsClosed()) &&
-      !FinishedReadingTrailers()) {
-    // Before QPack, trailers can arrive before body. Only decode trailers after finishing decoding
-    // body.
+  if (session()->connection()->connected() && !rst_sent()) {
+    maybeDecodeTrailers();
+  }
+}
+
+void EnvoyQuicClientStream::maybeDecodeTrailers() {
+  if (sequencer()->IsClosed() && !FinishedReadingTrailers()) {
+    ASSERT(!received_trailers().empty());
+    // Only decode trailers after finishing decoding body.
     response_decoder_->decodeTrailers(
         spdyHeaderBlockToEnvoyHeaders<Http::ResponseTrailerMapImpl>(received_trailers()));
+    end_stream_decoded_ = true;
     MarkTrailersConsumed();
   }
 }
 
 void EnvoyQuicClientStream::OnStreamReset(const quic::QuicRstStreamFrame& frame) {
   quic::QuicSpdyClientStream::OnStreamReset(frame);
-  runResetCallbacks(quicRstErrorToEnvoyResetReason(frame.error_code));
+  runResetCallbacks(quicRstErrorToEnvoyRemoteResetReason(frame.error_code));
+}
+
+void EnvoyQuicClientStream::Reset(quic::QuicRstStreamErrorCode error) {
+  // Upper layers expect calling resetStream() to immediately raise reset callbacks.
+  runResetCallbacks(quicRstErrorToEnvoyLocalResetReason(error));
+  quic::QuicSpdyClientStream::Reset(error);
 }
 
 void EnvoyQuicClientStream::OnConnectionClosed(quic::QuicErrorCode error,

source/extensions/quic_listeners/quiche/envoy_quic_client_stream.h

@@ -46,6 +46,7 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream,
   // quic::QuicSpdyStream
   void OnBodyAvailable() override;
   void OnStreamReset(const quic::QuicRstStreamFrame& frame) override;
+  void Reset(quic::QuicRstStreamErrorCode error) override;
   void OnClose() override;
   void OnCanWrite() override;
   // quic::Stream
@@ -67,6 +68,9 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream,
 private:
   QuicFilterManagerConnectionImpl* filterManagerConnection();
 
+  // Deliver awaiting trailers if body has been delivered.
+  void maybeDecodeTrailers();
+
   Http::ResponseDecoder* response_decoder_{nullptr};
 };
 

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc

@@ -84,6 +84,10 @@ void EnvoyQuicServerStream::encodeHeaders(const Http::ResponseHeaderMap& headers
 void EnvoyQuicServerStream::encodeData(Buffer::Instance& data, bool end_stream) {
   ENVOY_STREAM_LOG(debug, "encodeData (end_stream={}) of {} bytes.", *this, end_stream,
                    data.length());
+  if (data.length() == 0 && !end_stream) {
+    return;
+  }
+  ASSERT(!local_end_stream_);
   local_end_stream_ = end_stream;
   // This is counting not serialized bytes in the send buffer.
   const uint64_t bytes_to_send_old = BufferedDataBytes();
@@ -121,8 +125,6 @@ void EnvoyQuicServerStream::encodeMetadata(const Http::MetadataMapVector& /*meta
 }
 
 void EnvoyQuicServerStream::resetStream(Http::StreamResetReason reason) {
-  // Upper layers expect calling resetStream() to immediately raise reset callbacks.
-  runResetCallbacks(reason);
   if (local_end_stream_ && !reading_stopped()) {
     // This is after 200 early response. Reset with QUIC_STREAM_NO_ERROR instead
     // of propagating original reset reason. In QUICHE if a stream stops reading
@@ -146,10 +148,17 @@ void EnvoyQuicServerStream::switchStreamBlockState(bool should_block) {
 
 void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len,
                                                      const quic::QuicHeaderList& header_list) {
+  // TODO(danzh) Fix in QUICHE. If the stream has been reset in the call stack,
+  // OnInitialHeadersComplete() shouldn't be called.
+  if (rst_sent()) {
+    return;
+  }
   quic::QuicSpdyServerStreamBase::OnInitialHeadersComplete(fin, frame_len, header_list);
-  ASSERT(headers_decompressed());
+  ASSERT(headers_decompressed() && !header_list.empty());
+
   request_decoder_->decodeHeaders(
-      quicHeadersToEnvoyHeaders<Http::RequestHeaderMapImpl>(header_list), /*end_stream=*/fin);
+      quicHeadersToEnvoyHeaders<Http::RequestHeaderMapImpl>(header_list),
+      /*end_stream=*/fin);
   if (fin) {
     end_stream_decoded_ = true;
   }
@@ -179,17 +188,15 @@ void EnvoyQuicServerStream::OnBodyAvailable() {
     MarkConsumed(bytes_read);
   }
 
-  // True if no trailer and FIN read.
-  bool finished_reading = IsDoneReading();
-  bool empty_payload_with_fin = buffer->length() == 0 && fin_received();
+  bool fin_read_and_no_trailers = IsDoneReading();
   // If this call is triggered by an empty frame with FIN which is not from peer
   // but synthesized by stream itself upon receiving HEADERS with FIN or
   // TRAILERS, do not deliver end of stream here. Because either decodeHeaders
   // already delivered it or decodeTrailers will be called.
-  bool skip_decoding = empty_payload_with_fin && (end_stream_decoded_ || !finished_reading);
+  bool skip_decoding = (buffer->length() == 0 && !fin_read_and_no_trailers) || end_stream_decoded_;
   if (!skip_decoding) {
-    request_decoder_->decodeData(*buffer, finished_reading);
-    if (finished_reading) {
+    request_decoder_->decodeData(*buffer, fin_read_and_no_trailers);
+    if (fin_read_and_no_trailers) {
       end_stream_decoded_ = true;
     }
   }
@@ -204,35 +211,43 @@ void EnvoyQuicServerStream::OnBodyAvailable() {
     return;
   }
 
-  if (!quic::VersionUsesHttp3(transport_version()) && !FinishedReadingTrailers()) {
-    // For Google QUIC implementation, trailers may arrived earlier and wait to
-    // be consumed after reading all the body. Consume it here.
-    // IETF QUIC shouldn't reach here because trailers are sent on same stream.
-    request_decoder_->decodeTrailers(
-        spdyHeaderBlockToEnvoyHeaders<Http::RequestTrailerMapImpl>(received_trailers()));
-    MarkTrailersConsumed();
-  }
+  // Trailers may arrived earlier and wait to be consumed after reading all the body. Consume it
+  // here.
+  maybeDecodeTrailers();
+
   OnFinRead();
   in_decode_data_callstack_ = false;
 }
 
 void EnvoyQuicServerStream::OnTrailingHeadersComplete(bool fin, size_t frame_len,
                                                       const quic::QuicHeaderList& header_list) {
   quic::QuicSpdyServerStreamBase::OnTrailingHeadersComplete(fin, frame_len, header_list);
-  if (session()->connection()->connected() &&
-      (quic::VersionUsesHttp3(transport_version()) || sequencer()->IsClosed()) &&
-      !FinishedReadingTrailers()) {
-    // Before QPack trailers can arrive before body. Only decode trailers after finishing decoding
-    // body.
+  ASSERT(trailers_decompressed());
+  if (session()->connection()->connected() && !rst_sent()) {
+    maybeDecodeTrailers();
+  }
+}
+
+void EnvoyQuicServerStream::maybeDecodeTrailers() {
+  if (sequencer()->IsClosed() && !FinishedReadingTrailers()) {
+    ASSERT(!received_trailers().empty());
+    // Only decode trailers after finishing decoding body.
     request_decoder_->decodeTrailers(
         spdyHeaderBlockToEnvoyHeaders<Http::RequestTrailerMapImpl>(received_trailers()));
+    end_stream_decoded_ = true;
     MarkTrailersConsumed();
   }
 }
 
 void EnvoyQuicServerStream::OnStreamReset(const quic::QuicRstStreamFrame& frame) {
   quic::QuicSpdyServerStreamBase::OnStreamReset(frame);
-  runResetCallbacks(quicRstErrorToEnvoyResetReason(frame.error_code));
+  runResetCallbacks(quicRstErrorToEnvoyRemoteResetReason(frame.error_code));
+}
+
+void EnvoyQuicServerStream::Reset(quic::QuicRstStreamErrorCode error) {
+  // Upper layers expect calling resetStream() to immediately raise reset callbacks.
+  runResetCallbacks(quicRstErrorToEnvoyLocalResetReason(error));
+  quic::QuicSpdyServerStreamBase::Reset(error);
 }
 
 void EnvoyQuicServerStream::OnConnectionClosed(quic::QuicErrorCode error,

source/extensions/quic_listeners/quiche/envoy_quic_server_stream.h

@@ -49,9 +49,10 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase,
   // quic::QuicSpdyStream
   void OnBodyAvailable() override;
   void OnStreamReset(const quic::QuicRstStreamFrame& frame) override;
+  void Reset(quic::QuicRstStreamErrorCode error) override;
   void OnClose() override;
   void OnCanWrite() override;
-  // quic::QuicServerSessionBase
+  // quic::QuicSpdyServerStreamBase
   void OnConnectionClosed(quic::QuicErrorCode error, quic::ConnectionCloseSource source) override;
 
 protected:
@@ -69,6 +70,9 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase,
 private:
   QuicFilterManagerConnectionImpl* filterManagerConnection();
 
+  // Deliver awaiting trailers if body has been delivered.
+  void maybeDecodeTrailers();
+
   Http::RequestDecoder* request_decoder_{nullptr};
 };
 

source/extensions/quic_listeners/quiche/envoy_quic_utils.cc

@@ -65,20 +65,32 @@ quic::QuicRstStreamErrorCode envoyResetReasonToQuicRstError(Http::StreamResetRea
   case Http::StreamResetReason::LocalRefusedStreamReset:
     return quic::QUIC_REFUSED_STREAM;
   case Http::StreamResetReason::ConnectionFailure:
+  case Http::StreamResetReason::ConnectionTermination:
     return quic::QUIC_STREAM_CONNECTION_ERROR;
   case Http::StreamResetReason::LocalReset:
     return quic::QUIC_STREAM_CANCELLED;
-  case Http::StreamResetReason::ConnectionTermination:
-    return quic::QUIC_STREAM_NO_ERROR;
   default:
     return quic::QUIC_BAD_APPLICATION_PAYLOAD;
   }
 }
 
-Http::StreamResetReason quicRstErrorToEnvoyResetReason(quic::QuicRstStreamErrorCode rst_err) {
+Http::StreamResetReason quicRstErrorToEnvoyLocalResetReason(quic::QuicRstStreamErrorCode rst_err) {
+  switch (rst_err) {
+  case quic::QUIC_REFUSED_STREAM:
+    return Http::StreamResetReason::LocalRefusedStreamReset;
+  case quic::QUIC_STREAM_CONNECTION_ERROR:
+    return Http::StreamResetReason::ConnectionFailure;
+  default:
+    return Http::StreamResetReason::LocalReset;
+  }
+}
+
+Http::StreamResetReason quicRstErrorToEnvoyRemoteResetReason(quic::QuicRstStreamErrorCode rst_err) {
   switch (rst_err) {
   case quic::QUIC_REFUSED_STREAM:
     return Http::StreamResetReason::RemoteRefusedStreamReset;
+  case quic::QUIC_STREAM_CONNECTION_ERROR:
+    return Http::StreamResetReason::ConnectError;
   default:
     return Http::StreamResetReason::RemoteReset;
   }

source/extensions/quic_listeners/quiche/envoy_quic_utils.h

@@ -66,7 +66,10 @@ spdy::SpdyHeaderBlock envoyHeadersToSpdyHeaderBlock(const Http::HeaderMap& heade
 quic::QuicRstStreamErrorCode envoyResetReasonToQuicRstError(Http::StreamResetReason reason);
 
 // Called when a RST_STREAM frame is received.
-Http::StreamResetReason quicRstErrorToEnvoyResetReason(quic::QuicRstStreamErrorCode rst_err);
+Http::StreamResetReason quicRstErrorToEnvoyLocalResetReason(quic::QuicRstStreamErrorCode rst_err);
+
+// Called when a QUIC stack reset the stream.
+Http::StreamResetReason quicRstErrorToEnvoyRemoteResetReason(quic::QuicRstStreamErrorCode rst_err);
 
 // Called when underlying QUIC connection is closed either locally or by peer.
 Http::StreamResetReason quicErrorCodeToEnvoyResetReason(quic::QuicErrorCode error);

test/extensions/quic_listeners/quiche/BUILD

@@ -84,6 +84,7 @@ envoy_cc_test(
         "//test/mocks/network:network_mocks",
         "//test/test_common:utility_lib",
         "@com_googlesource_quiche//:quic_core_http_spdy_session_lib",
+        "@com_googlesource_quiche//:quic_test_tools_qpack_qpack_test_utils_lib",
         "@com_googlesource_quiche//:quic_test_tools_session_peer_lib",
     ],
 )
@@ -105,6 +106,7 @@ envoy_cc_test(
         "//test/mocks/network:network_mocks",
         "//test/test_common:utility_lib",
         "@com_googlesource_quiche//:quic_core_http_spdy_session_lib",
+        "@com_googlesource_quiche//:quic_test_tools_qpack_qpack_test_utils_lib",
     ],
 )
 
@@ -290,5 +292,6 @@ envoy_cc_test_library(
         "//test/test_common:environment_lib",
         "@com_googlesource_quiche//:quic_core_http_spdy_session_lib",
         "@com_googlesource_quiche//:quic_test_tools_first_flight_lib",
+        "@com_googlesource_quiche//:quic_test_tools_qpack_qpack_encoder_test_utils_lib",
     ],
 )