net: add missing return value in SSL_SetNextProtoCallback

net/third_party/nss/ssl/ssl3con.c

 /*
  * SSL3 Protocol
  *
  * ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
  * 1.1 (the "License"); you may not use this file except in compliance with
  * the License. You may obtain a copy of the License at
  * http://www.mozilla.org/MPL/
@@ skipping 5881 matching lines @@
         rv = ssl3_SendCertificateVerify(ss);
         if (rv != SECSuccess) {
             goto loser; /* err is set. */
         }
     }
     rv = ssl3_SendChangeCipherSpecs(ss);
     if (rv != SECSuccess) {
         goto loser;     /* err code was set. */
     }
 
-    rv = ssl3_SendNextProto(ss);
-    if (rv != SECSuccess) {
-        goto loser;     /* err code was set. */
+    if (!ss->firstHsDone) {

[wtc, 2011/10/28 18:22:05]

We should add a comment to explain this, because the reason
is not obvious.  I guess this is because the next protocol
is not part of the SSL session state, so it is irrelevant
when renegotiating the SSL session parameters.

[agl, 2011/10/28 19:15:18]

Done.

+        rv = ssl3_SendNextProto(ss);
+        if (rv != SECSuccess) {
+            goto loser; /* err code was set. */
+        }
     }
 
     rv = ssl3_SendFinished(ss, 0);
     if (rv != SECSuccess) {
         goto loser;     /* err code was set. */
     }
 
     ssl_ReleaseXmitBufLock(ss);         /*******************************/
 
     if (ssl3_ExtensionNegotiated(ss, ssl_session_ticket_xtn))
@@ skipping 2914 matching lines @@
         /* If this thread is in SSL_SecureSend (trying to write some data) 
         ** or if it is going to step up, 
         ** then set the ssl_SEND_FLAG_FORCE_INTO_BUFFER flag, so that the 
         ** last two handshake messages (change cipher spec and finished) 
         ** will be sent in the same send/write call as the application data.
         */
         if (doStepUp || ss->writerThread == PR_GetCurrentThread()) {
             flags = ssl_SEND_FLAG_FORCE_INTO_BUFFER;
         }
 
-        if (!isServer) {
+        if (!isServer && !ss->firstHsDone) {
             rv = ssl3_SendNextProto(ss);
             if (rv != SECSuccess) {
                 goto xmit_loser;        /* err code was set. */
             }
         }

[wtc, 2011/10/28 18:22:05]

It seems that this whole thing can be removed.  I guess this
is for the session resumption case, because the next protocol
by definition is not part of the SSL session state, and so
still needs to be negotiated in a session resumption handshake?

[agl, 2011/10/28 19:15:18]

Right, we explicitly say that NPN isn't part of the session state.

 
         rv = ssl3_SendFinished(ss, flags);
         if (rv != SECSuccess) {
             goto xmit_loser;    /* err is set. */
         }
     }
 
     /* Optimization: don't cache this connection if we're going to step up. */
     if (doStepUp) {
         ssl_FreeSID(sid);
@@ skipping 1138 matching lines @@
 
     ss->ssl3.initialized = PR_FALSE;
 
     if (ss->ssl3.nextProto.data) {
         PORT_Free(ss->ssl3.nextProto.data);
         ss->ssl3.nextProto.data = NULL;
     }
 }
 
 /* End of ssl3con.c */