66706 - ssl_Poll mishandles PR_POLL_READ on brand new client connection

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P3)

Description

[John G. Myers]

This is a subset of bug 56924.  On a connection on which handshake-as-client 
has been started but the initial handshake record has not been sent, 
PR_POLL_READ will incorrectly wait for data on the underlying socket.

Comment 1

[John G. Myers]

Proposed fix is http://bugzilla.mozilla.org/showattachment.cgi?attach_id=23017

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Alternative proposed patch.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comments on this new proposed patch.

Besides the obvious difference of factoring out a 
common subexpression from the two large ifs to make
if (ss->useSecurity && !ss->connected) {
this patch also returns that the socket is readable,
even when the caller has also polled on write.

The previous patch would not return that it was 
immediately readable if it was also polling on 
write.  

John, do you agree with this change in the logic?

Comment 4

[John G. Myers]

I agree with the change in logic.  However, I don't think the code correctly 
handles the case where ret_flags == PR_POLL_WRITE && ss->handshake == 
ssl2_BeginClientHandshake.  In that case it should either poll the underlying 
socket for write or immediately set out_flags to PR_POLL_WRITE.

Perhaps we should

if (useSecurity && !connected) {
   if (handshake == ssl2_BeginClientHandshake) {
      *outflags = ret_flags;
      return ret_flags;
   }
   if ((ret_flags & PR_POLL_WRITE)...

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Another proposed patch will come along shortly.
Setting target milestone to 3.2

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Another patch, includes partial fix for 56924

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

The correct fix for this requires knowledge of whether or
not the socket is connected. I cannot complette this work 
today, so this bug will be deferred until NSS 3.2.1, at least.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I recently rewrote the ssl_Poll function.  I believe the new
version fixes bug 66706.  It is checked in on the trunk and
will be part of NSS 3.3.

John, will you verify this?

Comment 9

[John G. Myers]

I am unable to verify this as I cannot compile PSM2 against the tip:

gmake[2]: Entering directory `/misc/jgmyers/mozilla/security/nss/lib/pk11wrap'
gcc -o Linux2.2_x86_glibc_PTH_DBG.OBJ/pk11cert.o -c -g -fPIC -DLINUX1_2 -Di386 
-D_XOPEN_SOURCE -DLINUX2_1 -ansi -Wall -pipe -DLINUX -Dlinux -D_POSIX_SOURCE 
-D_BSD_SOURCE -DHAVE_STRERROR -DXP_UNIX -DDEBUG -UNDEBUG -DDEBUG_jgmyers 
-D_REENTRANT -I/misc/jgmyers/mozilla/dist/include/nspr  
-I../../../../dist/public/security -I../../../../dist/private/security 
-I../../../../dist/public/security -I../../../../dist/public/dbm  pk11cert.c
In file included from pk11cert.c:40:
secmodi.h:41: mcom_db.h: No such file or directory

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Sounds like you need to update coreconf

Comment 11

[Sonja Mirtitsch]

cvs update -P
read the troubleshooting page...

Comment 12

[John G. Myers]

Verified.