commit e3e93d8c9b5d202ab3f75c3b86cc6979732d60c8
author wub <wub@google.com> Wed Jun 01 14:51:43 2022 -0700
committer Copybara-Service <copybara-worker@google.com> Wed Jun 01 14:54:46 2022 -0700
tree 06121c8264d00355bd6069f7ca0bcb6bd5a7f115
parent 2ffa68ff73edaa6b735039879605fbf37bf9cf2a

Add an early return in TlsHandshaker::AdvanceHandshake if connection is closed after SSL_do_handshake.

This is intended to fix https://bugs.chromium.org/p/chromium/issues/detail?id=1305526, however we don't have hard evidence to show when the connection is closed in the bug.

Protected by FLAGS_quic_reloadable_flag_quic_tls_handshaker_check_connection_closed.

PiperOrigin-RevId: 452390558

quiche/quic/core/quic_flags_list.h

diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h
index 62407d4..916d885 100644
--- a/quiche/quic/core/quic_flags_list.h
+++ b/quiche/quic/core/quic_flags_list.h
@@ -23,6 +23,8 @@
 QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_enable_mtu_discovery_at_server, false)
 // If true, QuicGsoBatchWriter will support release time if it is available and the process has the permission to do so.
 QUIC_FLAG(FLAGS_quic_restart_flag_quic_support_release_time_for_gso, false)
+// If true, TlsHandshaker::AdvanceHandshake will check if connection is closed after SSL_do_handshake.
+QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_tls_handshaker_check_connection_closed, true)
 // If true, abort async QPACK header decompression in QuicSpdyStream::Reset() and in QuicSpdyStream::OnStreamReset().
 QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_abort_qpack_on_stream_reset, true)
 // If true, ack frequency frame can be sent from server to client.

quiche/quic/core/tls_handshaker.cc

diff --git a/quiche/quic/core/tls_handshaker.cc b/quiche/quic/core/tls_handshaker.cc
index 15b4d4f..0449c67 100644
--- a/quiche/quic/core/tls_handshaker.cc
+++ b/quiche/quic/core/tls_handshaker.cc
@@ -85,7 +85,7 @@
 }
 
 void TlsHandshaker::AdvanceHandshake() {
-  if (is_connection_closed_) {
+  if (is_connection_closed()) {
     return;
   }
   if (GetHandshakeState() >= HANDSHAKE_COMPLETE) {
@@ -101,6 +101,13 @@
   QUIC_VLOG(1) << ENDPOINT << "Continuing handshake";
   int rv = SSL_do_handshake(ssl());
 
+  if (GetQuicReloadableFlag(quic_tls_handshaker_check_connection_closed) &&
+      is_connection_closed()) {
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_tls_handshaker_check_connection_closed, 1,
+                                 2);
+    return;
+  }
+
   // If SSL_do_handshake return success(1) and we are in early data, it is
   // possible that we have provided ServerHello to BoringSSL but it hasn't been
   // processed. Retry SSL_do_handshake once will advance the handshake more in
@@ -109,6 +116,14 @@
   if (rv == 1 && SSL_in_early_data(ssl())) {
     OnEnterEarlyData();
     rv = SSL_do_handshake(ssl());
+
+    if (GetQuicReloadableFlag(quic_tls_handshaker_check_connection_closed) &&
+        is_connection_closed()) {
+      QUIC_RELOADABLE_FLAG_COUNT_N(quic_tls_handshaker_check_connection_closed,
+                                   2, 2);
+      return;
+    }
+
     QUIC_VLOG(1) << ENDPOINT
                  << "SSL_do_handshake returned when entering early data. After "
                  << "retry, rv=" << rv
@@ -120,7 +135,7 @@
     //   SSL_in_early_data should be false.
     //
     // In either case, it should not both return 1 and stay in early data.
-    if (rv == 1 && SSL_in_early_data(ssl()) && !is_connection_closed_) {
+    if (rv == 1 && SSL_in_early_data(ssl()) && !is_connection_closed()) {
       QUIC_BUG(quic_handshaker_stay_in_early_data)
           << "The original and the retry of SSL_do_handshake both returned "
              "success and in early data";
@@ -139,7 +154,7 @@
     return;
   }
   if (ShouldCloseConnectionOnUnexpectedError(ssl_error) &&
-      !is_connection_closed_) {
+      !is_connection_closed()) {
     QUIC_VLOG(1) << "SSL_do_handshake failed; SSL_get_error returns "
                  << ssl_error;
     ERR_print_errors_fp(stderr);