http://mxr.mozilla.org/security/source/security/nss/lib/ssl/ssl3gthr.c#207 207 /* Treat an empty msgState like a NULL msgState. (Most of the time 208 * when ssl3_HandleHandshake returns SECWouldBlock, it leaves 209 * behind a non-NULL but zero-length msgState). 210 * Test: async_cert_restart_server_sends_hello_request_first_in_separate_record 211 */ 212 if (ss->ssl3.hs.msgState.buf != NULL) { 213 if (ss->ssl3.hs.msgState.len == 0) { 214 ss->ssl3.hs.msgState.buf = NULL; 215 } 216 } 217 218 if (ss->ssl3.hs.msgState.buf != NULL) { 219 /* ssl3_HandleHandshake previously returned SECWouldBlock and the 220 * as-yet-unprocessed plaintext of that previous handshake record. 221 * We need to process it now before we overwrite it with the next 222 * handshake record. 223 */ 224 rv = ssl3_HandleRecord(ss, NULL, &ss->gs.buf); It seems like the accessed to ss->ssl3.hs.msgState should be protected by the SSL3 handshake lock. ssl3_RestartHandshakeAfterCertReq has the same problem. This is a problem that was identified by Wan-Teh and EKR.

Comment on attachment 745475 [details] [diff] [review] Hold lock across all uses of ssl3.hs in ssl3_GatherCompleteHandshake Review of attachment 745475 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. ::: lib/ssl/ssl3gthr.c @@ +286,1 @@ > /* Without this, we may end up wrongly reporting Do you know what "this" refers to? It seems to refer to the ss->ssl3.hs.restartTarget == NULL test. If so, I suggest moving this comment after ssl_GetSSL3HandshakeLock(ss), and also changing "this" to "this check" or "this test". @@ +294,3 @@ > if (rv != SECSuccess) { > PORT_SetError(PR_WOULD_BLOCK_ERROR); > + ssl_ReleaseSSL3HandshakeLock(ss); Call PORT_SetError() after releasing the lock. We should do as little work as possible while holding a lock to avoid lock contention.

Comment on attachment 745475 [details] [diff] [review] Hold lock across all uses of ssl3.hs in ssl3_GatherCompleteHandshake https://hg.mozilla.org/projects/nss/rev/753e9555853d Thanks for the review. I made the changes you suggested in this changeset.

Besides the changes you suggested, I found that there are further clarifications that should be made.

Comment on attachment 745615 [details] [diff] [review] Clarify logic in ssl3_GatherCompleteHandshake regarding async. cert verification Review of attachment 745615 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. ::: lib/ssl/ssl3gthr.c @@ +304,4 @@ > if (ss->ssl3.hs.msgState.len == 0) { > ss->ssl3.hs.msgState.buf = NULL; > + } else { > + handleRecordNow = PR_TRUE; We can also just test ss->ssl3.hs.msgState.len != 0 and leave a (buf=non-null, len=0) combination unchanged.

Comment on attachment 745615 [details] [diff] [review] Clarify logic in ssl3_GatherCompleteHandshake regarding async. cert verification (In reply to Wan-Teh Chang from comment #6) > Comment on attachment 745615 [details] [diff] [review] > Clarify logic in ssl3_GatherCompleteHandshake regarding async. cert > verification > > Review of attachment 745615 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=wtc. > > ::: lib/ssl/ssl3gthr.c > @@ +304,4 @@ > > if (ss->ssl3.hs.msgState.len == 0) { > > ss->ssl3.hs.msgState.buf = NULL; > > + } else { > > + handleRecordNow = PR_TRUE; > > We can also just test ss->ssl3.hs.msgState.len != 0 and > leave a (buf=non-null, len=0) combination unchanged. Thanks for the review Wan-Teh. I think your suggested change makes sense, but I did not make that change because it would have taken too much time to convince myself that it is definitely OK. http://hg.mozilla.org/projects/nss/rev/cf3b9b27172e