Closed Bug 676729 Opened 13 years ago Closed 12 years ago
_Force Handshake does not send the saved write data in ss->pending Buf
This bug was originally reported in Chromium bug 91458: http://code.google.com/p/chromium/issues/detail?id=91458 If a SSL_ForceHandshake call cannot send all the data to the lower layer, the unsent write data is saved in ss->pendingBuf. This can happen when an SSL client sends a big Certificate message that cannot be consumed by the lower layer at once. When the lower layer becomes writable, we call SSL_ForceHandshake again. But the saved write data in ss->pendingBuf is not sent, so the SSL server will not send the ChangeCipherSpec and Finished messages. Eventually the server closes the connection, or the client times out. The proposed patch fixes this by copying some code from ssl_SecureRecv that handles a similar problem. I didn't copy the !ss->opt.fdx test to SSL_ForceHandshake because it is irrelevant to SSL_ForceHandshake. I also removed the "XXX short write?" comment because I think short write should be handled in the same way as the PR_WOULD_BLOCK_ERROR: it is not an error and can be ignored.
Nelson: I think the !ss->opt.fdx test may need to be removed from ssl_SecureRecv to avoid this problem during a renegotiation. In a web browser, SSL renegotiation usually occurs while the browser is reading an HTTP response. So ssl_SecureRecv drives the renegotiation. The browser doesn't need to set the SSL_ENABLE_FDX option.
Comment on attachment 550890 [details] [diff] [review] Proposed patch r+ rrelyea I agreee fdx is not a factor in SSL_ForceHandshake().
Attachment #550890 - Flags: review?(rrelyea) → review+
Comment on attachment 550890 [details] [diff] [review] Proposed patch I will do a review of this in case Nelson cannot get to it.
Patch checked in on the NSS trunk (NSS 3.13.2). Checking in sslsecur.c; /cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v <-- sslsecur.c new revision: 1.50; previous revision: 1.49 done The remaining issue is what I described in comment 2: the !ss->opt.fdx test may need to be removed from ssl_SecureRecv: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslsecur.c&rev=1.49&mark=1120,1136#1119
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: 3.13.1 → 3.13.2
Wan-Teh, if ssl_SendSavedWriteData(ss) returns (SECWouldBlock) and there is still data in ss->pendingBuf, shouldn't we return SECWouldBlock? Otherwise, it seems like we might write part of ss->pendingBuf but not all of it. Is there some reason that can't happen?
Brian: the answer to your question can be found by code inspection. ======================================================================== 1. ssl_SendSavedWriteData can return either 0 (= SECSuccess) or the return value of ssl_DefSend. 2. ssl_DefSend can return either |sent|, SECFailure, or |rv|. |sent| is an int initialized to 0 and only incremented by a nonnegative (>= 0) value. So |sent| is >= 0. SECFailure is -1. |rv| is the return value of an NSPR sent I/O method. So |rv| is either -1 or a nonnegative number (>= 0). ssl_DefSend returns |rv| only if |rv| is < 0. So, ssl_DefSend can return either -1 or a nonnegative value (>= 0). This shows that ssl_SendSavedWriteData cannot return SECWouldBlock (-2). ======================================================================== ssl_SendSavedWriteData reports the "would block" error by returning -1 with error code PR_WOULD_BLOCK_ERROR. When that happens, SSL_ForceHandshake handles it as a partial write (of 0 bytes) and keeps going rather than failing. In other words, the ssl_SendSavedWriteData(ss) call is a best-effort attempt to send as much pending data as we can. I think this behavior is correct.
You need to log in before you can comment on or make changes to this bug.