132899 - ssl_SecureSend sets PR_WOULD_BLOCK_ERROR after partial write

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Ian McGreer]

The following code occurs in ssl_SecureSend():

http://lxr.mozilla.org/mozilla/source/security/nss/lib/ssl/sslsecur.c#1002

1015 ssl_GetXmitBufLock(ss);
1016 if (ss->pendingBuf.len != 0) {
1017  PORT_Assert(ss->pendingBuf.len > 0);
1018  rv = ssl_SendSavedWriteData(ss, &ss->pendingBuf, ssl_DefSend);
1019  if (ss->pendingBuf.len != 0) {
1020   PORT_Assert(ss->pendingBuf.len > 0);
1021   PORT_SetError(PR_WOULD_BLOCK_ERROR);

This should not be PR_WOULD_BLOCK_ERROR, but a real error.  Or, perhaps, it
should only be PR_WOULD_BLOCK_ERROR if rv < 0.

1022   rv = SECFailure;
1023  }
1024 }
1025 ssl_ReleaseXmitBufLock(ss);
1026 if (rv < 0) {
1027  return rv;
1028 }

Comment 1

[Ian McGreer]

This is based on what someone told me.  The full description is that a client
closes a connection, and the server loops forever trying to send to a broken
socket.  The user noticed that PR_Writev is returning -1 with
PR_WOULD_BLOCK_ERROR, causing the caller to continue to poll.

Comment 2

[Ian McGreer]

patch checked in.

I will try to have the customer test this ASAP.

Comment 3

[Ian McGreer]

comment #2 was not meant for this bug.

After discussing with Wan-Teh, we believe this code is correct.  The user is
most likely seeing incorrect behavior from bug 132889, not a problem in
ssl_SecureSend.  Marking invalid.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

This code looks wrong to me.  

If this is a non-blocking socket, and there was data left unsent by 
a previous write attempt, then ss->pendingBuf.len will be != 0 on line
1016.  Then  ssl_SecureSend calls ssl_SendSavedWriteData which calls
ssl_DefSend to attempt the write.  

There are 4 cases for the values returned by ssl_DefSend:
a) rv == len,  it sent all the data.
b) 0 < rv < len, it sent some but not all data.  
c) rv < 0, err == PR_WOULD_BLOCK_ERROR
d) rv < 0, some other error

ssl_SendSavedWriteData returns the same value returned by ssl_DefSend.

ssl_SecureSend wants to return SECFailure with error PR_WOULD_BLOCK_ERROR
in cases b and c above, but not in case d.  Currently the code returns
SECFailure with PR_WOULD_BLOCK_ERROR for cases b, c and d.  

So, I think the correction is to change the test following the call to
ssl_SendSavedWriteData() from 

    if (ss->pendingBuf.len != 0) {

to 

    if (rv >= 0 && ss->pendingBuf.len != 0) {

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

reassigning to myself.

Comment 6

[Wan-Teh Chang]

r=wtc.  Thank you, Nelson, for catching this error
that Ian and I missed :-)

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Checking in sslsecur.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v  <--  sslsecur.c
new revision: 1.17; previous revision: 1.16
done

Comment 8

[Ian McGreer]

Attachment: patch for 3.3 branch

for posterity, this patch has the bugfix, and was the one used for the 3.3
branch.

Comment 9

[Ian McGreer]

The fix for this bug was rolled back into 3.3.3.