Make SSL False Start work with asynchronous certificate validation

net/third_party/nss/ssl/ssl3gthr.c

Index: net/third_party/nss/ssl/ssl3gthr.c
===================================================================
--- net/third_party/nss/ssl/ssl3gthr.c	(revision 227672)
+++ net/third_party/nss/ssl/ssl3gthr.c	(working copy)
@@ -275,12 +275,18 @@
 {
     SSL3Ciphertext cText;
     int            rv;
-    PRBool         canFalseStart = PR_FALSE;
+    PRBool         keepGoing = PR_TRUE;
 
     SSL_TRC(30, ("ssl3_GatherCompleteHandshake"));
 
+    /* ssl3_HandleRecord may end up eventually calling ssl_FinishHandshake,
+     * which requires the 1stHandshakeLock, which must be acquired before the
+     * RecvBufLock.
+     */
+    PORT_Assert( ss->opt.noLocks || ssl_Have1stHandshakeLock(ss) );
     PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) );
-    do {
+
+    for (;;) {

[briansmith, 2013/10/25 19:17:14]

The locking in this function was changed based on your request, so that it does
not hold the SSL3HandshakeLock during I/O.

[wtc, 2013/10/29 01:21:51]

This loop should be

    while (keepGoing) {

 	PRBool handleRecordNow = PR_FALSE;
 
 	ssl_GetSSL3HandshakeLock(ss);
@@ -364,24 +370,56 @@
 
 	    cText.buf     = &ss->gs.inbuf;
 	    rv = ssl3_HandleRecord(ss, &cText, &ss->gs.buf);
+
+	    if (rv == (int) SECSuccess && ss->gs.buf.len > 0) {
+		/* We have application data to return to the application. This
+		 * prioritizes returning application data to the application over
+		 * completing any renegotiation handshake we may be doing.

[wtc, 2013/10/29 19:54:22]

Unless you have seen this case in practice, I think this optimization for an
uncommon case is not worth the code complexity.

On the other hand, this case is specifically allowed by the comment on lines
258-259. It also seems to be in the original code. So I guess you are just
documenting an existing behavior?

[briansmith, 2013/10/29 21:00:13]

I am just documenting existing behavior. I don't want to change the behavior of
this in this patch.

+		 */
+		PORT_Assert(ss->firstHsDone);
+		PORT_Assert(cText.type == content_application_data);
+		break;
+	    }
 	}
 	if (rv < 0) {
 	    return ss->recvdCloseNotify ? 0 : rv;
 	}
 
-	/* If we kicked off a false start in ssl3_HandleServerHelloDone, break
-	 * out of this loop early without finishing the handshake.
-	 */
-	if (ss->opt.enableFalseStart) {
-	    ssl_GetSSL3HandshakeLock(ss);
-	    canFalseStart = (ss->ssl3.hs.ws == wait_change_cipher ||
-			     ss->ssl3.hs.ws == wait_new_session_ticket) &&
-		            ssl3_CanFalseStart(ss);
-	    ssl_ReleaseSSL3HandshakeLock(ss);
+	PORT_Assert(keepGoing);
+	ssl_GetSSL3HandshakeLock(ss);
+	if (ss->ssl3.hs.ws == idle_handshake) {

[briansmith, 2013/10/25 19:17:14]

Note that ss->ssl3.hs.ws needs to have the SSl3HandshakeLock held when it is
being accessed (AFAICT). Before this bug, ss->ssl3.hs.ws was being accessed
without the lock being held.

+	    /* We are done with the current handshake so stop trying to
+	     * handshake. Note that it would be safe to test ss->firstHsDone
+	     * instead of ss->ssl3.hs.ws. By testing ss->ssl3.hs.ws instead,
+	     * we prioritize completing a renegotiation handshake over sending
+	     * application data.
+	     */
+	    PORT_Assert(ss->firstHsDone);
+	    PORT_Assert(!ss->ssl3.hs.canFalseStart);
+	    keepGoing = PR_FALSE;
+	} else if (ss->ssl3.hs.canFalseStart) {
+	    /* Prioritize sending application data over trying to complete
+	     * the handshake if we're false starting.
+	     *
+	     * If we were to do this check at the beginning of the loop instead
+	     * of here, then SSL_ForceHandshake would become be a no-op after
+	     * receiving the ServerHelloDone. By doing this check here, we
+	     * ensure that SSL_ForceHandshake always tries to make a little
+	     * progress.

[wtc, 2013/10/29 19:54:22]

IMPORTANT: Chromium won't call SSL_ForceHandshake again after SSL_ForceHandshake
returns SECSuccess. Chromium will start calling PR_Send to send application
data.

So it doesn't seem necessary to mention SSL_ForceHandshake in this comment.
Moreover, if doing this check at the beginning of the loop allows you to
simplify the code, it may be worth doing. Otherwise, I don't think we need to
mention this. It seems to bring up an unimportant issue. I can confirm we didn't
deliberately put the check here.

[briansmith, 2013/10/29 21:00:13]

I am just trying to explain why, based on my explanation, things are the way
they are (the way they were before my patches). If we want to change how this
works, let's do that in another bug.

[briansmith, 2013/10/29 21:11:38]

I mean "based on my *investigation*."

Here is the the comment as I will check it in:

	    /* Prioritize sending application data over trying to complete
	     * the handshake if we're false starting.
	     *
	     * If we were to do this check at the beginning of the loop instead
	     * of here, then this function would become be a no-op after
	     * receiving the ServerHelloDone in the false start case. By doing
             * this check here, we ensure that we always try to make at least a
             * little progress. (XXX: It isn't clear whether is is actually
             * necessary for us to "make at least a little progress.")
	     */

[wtc, 2013/10/30 01:01:20]

is is => it is

But I think you should remove the "XXX" from the comment. In the false start
case, the handshake will be completed in ssl_SecureRecv (the DoRecv function),
so it is necessary for this function to call ssl3_GatherData and
ssl3_HandleRecord when ss->ssl3.hs.canFalseStart is true. So, it seems that we
will still exit this loop after making a little progress in that case, but
DoRecv -> ssl3_GatherAppDataRecord will immediately call this function again.

[briansmith, 2013/11/01 09:55:55]

Done.

+	     */
+	    PORT_Assert(!ss->firstHsDone);
+
+	    if (ssl3_WaitingForStartOfServerSecondRound(ss)) {
+		keepGoing = PR_FALSE;
+	    } else {
+		ss->ssl3.hs.canFalseStart = PR_FALSE;
+	    }

[briansmith, 2013/10/25 19:17:14]

I added this because we should not try to false start once we've received the
NewSessionTicket or change_cipher_spec, as explained in
ssl3_WaitingForStartOfServerSecondRound.

 	}
-    } while (ss->ssl3.hs.ws != idle_handshake &&
-             !canFalseStart &&
-             ss->gs.buf.len == 0);
+	ssl_ReleaseSSL3HandshakeLock(ss);
+	if (!keepGoing) {
+	    break;
+	}
+    }
 
     ss->gs.readOffset = 0;
     ss->gs.writeOffset = ss->gs.buf.len;
@@ -404,7 +442,10 @@
 {
     int            rv;
 
+    /* ssl3_GatherCompleteHandshake requires both of these locks. */
+    PORT_Assert( ss->opt.noLocks || ssl_Have1stHandshakeLock(ss) );
     PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) );
+
     do {
 	rv = ssl3_GatherCompleteHandshake(ss, flags);
     } while (rv > 0 && ss->gs.buf.len == 0);