tree 2c7bdc4a215c447ef60b1cdb6c430b88fd52ba24
parent 02703964432d6730ee5d51db48e9e2e9708d9f21
author fkastenholz <fkastenholz@google.com> 1573231825 -0800
committer Copybara-Service <copybara-worker@google.com> 1573231865 -0800

Handle case where crypto stream is not immediately available.

gfe-relnote: Not flag protected, fixes implementation bug in CL/275355012

First, CL/275355012 subtly changed how QuicSession reports whether the
handshake has been confirmed or not.

Prior to the CL, there was a bool in QuicSession,
is_handshake_confirmed_, that was initialized to false. When the
handshake completed/etc, the bool was flipped to true.  The failure
point that I ran across[1] used to test for handshake confirmation by
calling QuicSession::IsHandshakeConfirmed(), which merely returned the
value of the bool.

CL/275355012 changed that. First, it got rid of the bool &
QuicSession::IsHandshakeConfirmed(). Second, it changed
QuicSpdySession::SetMaxAllowedPushId. This method used to call
QuicSession::IsHandshakeConfirmed() to see if the handshake was done.
The CL changed it to call QuicSession::IsCryptoHandshakeConfirmed().

QuicSession::IsCryptoHandshakeConfirmed() was implemented as
   {
      return GetCryptoStream()->handshake_confirmed()
   }
So, if GetCryptoStream() returned NULL (as it would if called prior to
the crypto stream being set  up) then an addressing exception would occur.

Second

This happens because the failing Chromium tests all:
 1) Create a QuicChromiumClientSession
 2) Call <mumble>Session::Initialize()

<mumble>Session::Initialize() calls the internal method to create the
crypto stream.

But the QuicChromiumClientSession constructor calls
QuicSpdySession::SetMaxAllowedPushId(), which calls
QuicSession::IsCryptoHandshakeConfirmed() So the more detailed call
pattern is:

This happens because the failing Chromium tests all:
 1) Create a QuicChromiumClientSession
      Call QuicSpdySession::SetMaxAllowedPushId()
        Call QuicSession::IsCryptoHandshakeConfirmed()
          Call GetCryptoStream()
	    Dereference a null pointer
 2) Call <mumble>Session::Initialize()
      Set up the Crypto Stream.
        Set CryptoStream pointer to be a valid ptr.

This _used_ to work because the call pattern was
 1) Create a QuicChromiumClientSession
      Call QuicSpdySession::SetMaxAllowedPushId()
        Call QuicSession::IsHandshakeConfirmed()
          return the boolean
 2) Call <mumble>Session::Initialize()
      Set up the Crypto Stream.
          set the boolean true.

The fix is to first test the CryptoStream pointer. If it's null, it
means that the crypto stream was not set up yet, so the handshake can
not have occured, much less "be confirmed" and IsCryptoHandshakeConfirmed
returns fals. QED.

This is functionally identical to the state of things prior to
CL/275355012 because if the stream has not been created, the boolean
would still be false, and IsHandshakeConfirmed would have returned
false.

PiperOrigin-RevId: 279322611
Change-Id: I7edd34d0597809dc3086ae8eb940ef87f46d4d23