commit 9bcf307c4c58b624964a1af3812b86613a89de11
author David Benjamin <davidben@google.com> Mon Mar 29 17:43:03 2021 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Fri Apr 02 04:13:36 2021 +0000
tree ce4470a91b6f95346a9eb6affe94a9fd85ca821b
parent 0a6c3fc9c309f95408a3194f087b5394c198d250

Fix some unreachable code in the QUIC handshaker driver.

The check for ssl_hs_read_change_cipher_spec didn't do anything. Replace
it with an assert and add some comments since the hs->wait handling is a
little tricky.

Change-Id: I8e62ce3cceca9bed4611cb9d3faf0bfec3d3bdd4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46530
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

ssl/handshake.cc

diff --git a/ssl/handshake.cc b/ssl/handshake.cc
index b38f96a..d889372 100644
--- a/ssl/handshake.cc
+++ b/ssl/handshake.cc
@@ -552,7 +552,11 @@
 int ssl_run_handshake(SSL_HANDSHAKE *hs, bool *out_early_return) {
   SSL *const ssl = hs->ssl;
   for (;;) {
-    // Resolve the operation the handshake was waiting on.
+    // Resolve the operation the handshake was waiting on. Each condition may
+    // halt the handshake by returning, or continue executing if the handshake
+    // may immediately proceed. Cases which halt the handshake can clear
+    // |hs->wait| to re-enter the state machine on the next iteration, or leave
+    // it set to keep the condition sticky.
     switch (hs->wait) {
       case ssl_hs_error:
         ERR_restore_state(hs->error.get());
@@ -570,13 +574,13 @@
       case ssl_hs_read_message:
       case ssl_hs_read_change_cipher_spec: {
         if (ssl->quic_method) {
+          // QUIC has no ChangeCipherSpec messages.
+          assert(hs->wait != ssl_hs_read_change_cipher_spec);
+          // The caller should call |SSL_provide_quic_data|. Clear |hs->wait| so
+          // the handshake can check if there is sufficient data next iteration.
+          ssl->s3->rwstate = SSL_ERROR_WANT_READ;
           hs->wait = ssl_hs_ok;
-          // The change cipher spec is omitted in QUIC.
-          if (hs->wait != ssl_hs_read_change_cipher_spec) {
-            ssl->s3->rwstate = SSL_ERROR_WANT_READ;
-            return -1;
-          }
-          break;
+          return -1;
         }
 
         uint8_t alert = SSL_AD_DECODE_ERROR;
@@ -646,31 +650,30 @@
         return -1;
       }
 
+        // The following cases are associated with callback APIs which expect to
+        // be called each time the state machine runs. Thus they set |hs->wait|
+        // to |ssl_hs_ok| so that, next time, we re-enter the state machine and
+        // call the callback again.
       case ssl_hs_x509_lookup:
         ssl->s3->rwstate = SSL_ERROR_WANT_X509_LOOKUP;
         hs->wait = ssl_hs_ok;
         return -1;
-
       case ssl_hs_channel_id_lookup:
         ssl->s3->rwstate = SSL_ERROR_WANT_CHANNEL_ID_LOOKUP;
         hs->wait = ssl_hs_ok;
         return -1;
-
       case ssl_hs_private_key_operation:
         ssl->s3->rwstate = SSL_ERROR_WANT_PRIVATE_KEY_OPERATION;
         hs->wait = ssl_hs_ok;
         return -1;
-
       case ssl_hs_pending_session:
         ssl->s3->rwstate = SSL_ERROR_PENDING_SESSION;
         hs->wait = ssl_hs_ok;
         return -1;
-
       case ssl_hs_pending_ticket:
         ssl->s3->rwstate = SSL_ERROR_PENDING_TICKET;
         hs->wait = ssl_hs_ok;
         return -1;
-
       case ssl_hs_certificate_verify:
         ssl->s3->rwstate = SSL_ERROR_WANT_CERTIFICATE_VERIFY;
         hs->wait = ssl_hs_ok;