If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

SSLSocket.close() may be blocked and will not interrupt the SSLSocket.read() thread

RESOLVED FIXED in 4.2.5

Status

JSS
Library
P1
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: glen beasley, Assigned: glen beasley)

Tracking

4.2.4
4.2.5
x86
Mac OS X

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

11 years ago
Chee-weng Chea internally at Sun informed that in the current code base it is possible for SSLSocket.close to be blocked waiting on a SSLSocket.read operation.

   t@1                        t@2
-------------------------------------------------
      .                    lock readLock
      .                    lock SSLSocket(this)
      .                    what is isClosed? false
      .                    set inRead = true
      .                    unlock SSLSocket(this)
   lock SSLSocket(this)
   what is isClosed? false
   set isClosed = true
   inRead? true so
     abortReadWrite() == NOOP
   unlock SSLSocket(this)
      .                           .
      .                    read from SSLSocket (timeout=0)
      .                      "blocks until connection
      .                       timeout or disconnected)
      .                           .
      .                           . <-- Taking some time (NO RESPONSE)
      .
      .
   waiting readLock 

The socketRead() was started for t@4 despite the fact that t@6 flagged this socket to be closed(). t@6 did not interrupted the read operation since the native state reader/writer at the time was not set. The problem would have been addressed if the thread t@4 was able to detect that the isClosed flag had been set by T@6.
(Assignee)

Comment 1

11 years ago
sigh, I changed the id's of the thread 
> 
>    t@6                        t@4
> -------------------------------------------------
>       .                    lock readLock
>       .                    lock SSLSocket(this)
>       .                    what is isClosed? false
>       .                    set inRead = true
>       .                    unlock SSLSocket(this)
>    lock SSLSocket(this)
>    what is isClosed? false
>    set isClosed = true
>    inRead? true so
>      abortReadWrite() == NOOP
>    unlock SSLSocket(this)
>       .                           .
>       .                    read from SSLSocket (timeout=0)
>       .                      "blocks until connection
>       .                       timeout or disconnected)
>       .                           .
>       .                           . <-- Taking some time (NO RESPONSE)
>       .
>       .
>    waiting readLock 
> 
> The socketRead() was started for t@4 despite the fact that t@6 flagged this
> socket to be closed(). t@6 did not interrupted the read operation since the
> native state reader/writer at the time was not set. The problem would have been
> addressed if the thread t@4 was able to detect that the isClosed flag had been
> set by T@6.
> 

(Assignee)

Comment 2

11 years ago
Created attachment 261986 [details] [diff] [review]
if the socket is supposed to be closed don't try to read/write/accept

The fix needs to be done at the C layer.

Added a closePending boolean flag in JSSL_SocketData structure.

In abortReadWrite set closePending to PR_TRUE to signal the socket is to be closed. A thread trying to do a read/write operation will either not try to read/write if socket is marked as closedPending or be aborted by the thread that called abortReadWrite.

The same logic also needs to belong in the socketAccept and abortAccept for SSLServerSocket. 

Also removed some unused variables to reduce compiler warnings in the directory. 


   t@6                        t@4
-------------------------------------------------
      .                    lock readLock
      .                    lock SSLSocket(this)
      .                    what is isClosed? false
      .                    set inRead = true
      .                    unlock SSLSocket(this)
   lock SSLSocket(this)
   what is isClosed? false
   set isClosed = true
   inRead? true so
     abortReadWrite() == 
     JNI C layer
     lock(sock->Lock); 
     sock->closePending=TRUE
     unlock(sock->Lock);
   unlock SSLSocket(this)
                           JNI SocketRead
                           lock(sock->lock)
                           sock->closePending ==-TRUE
                           unlock(sock->lock) abort read throw exception
      .
   lock(reader) 
   lock(writer)
   close socket

or the case:
   
    t@6                        t@4
-------------------------------------------------
      .                    lock readLock
      .                    lock SSLSocket(this)
      .                    what is isClosed? false
      .                    set inRead = true
      .                    unlock SSLSocket(this)
      .                     JNI SocketRead
      .                          lock(sock->lock)
      .                          sock->closePending ==-false
      .                          sock->reader = currentThread
      .                          unlock(sock->lock) abort read throw exception
                                 ReadOperation occuring
   lock SSLSocket(this)
   what is isClosed? false
   set isClosed = true
   inRead? true so
     abortReadWrite() == 
     JNI C layer
     lock(sock->Lock); 
     sock->closePending=TRUE
     Abort I/O operations
     unlock(sock->Lock);
   unlock SSLSocket(this)
      .
   lock(reader) 
   lock(writer)
   close socket
Attachment #261986 - Flags: review?(wtc)

Comment 3

11 years ago
Comment on attachment 261986 [details] [diff] [review]
if the socket is supposed to be closed don't try to read/write/accept

There are several errors like this one:

>     /* Set the current thread doing the accept. */
>     me = PR_GetCurrentThread();
>     PR_Lock(sock->lock);
>+    if ( sock->closePending ) {
>+       PR_Unlock(sock->lock);
>+       JSSL_throwSSLSocketException(env, 
>+                "Accept operation failed socket is closing");
>+    }
>     PR_ASSERT(sock->accepter == NULL);
>     sock->accepter = me;
>     PR_Unlock(sock->lock);

I believe that you're missing a "goto finish" statement after
the JSSL_throwSSLSocketException call.  You can't continue
if sock->closePending is true.  Did you test this patch?
Attachment #261986 - Flags: review?(wtc) → review-
QA Contact: libraries
(Assignee)

Comment 4

11 years ago
Created attachment 262446 [details] [diff] [review]
tested patch...

if socket is supposed to be closed don't try to read/write/accept

I will submit a new test that I will add to all.pl in 
a separate patch.
Attachment #261986 - Attachment is obsolete: true
Attachment #262446 - Flags: review?(wtc)
(Assignee)

Updated

11 years ago
Priority: -- → P1

Comment 5

11 years ago
Comment on attachment 262446 [details] [diff] [review]
tested patch...

r=wtc.  Please make the following changes.

1. In SSLServerSocket.c

>     PR_Lock(sock->lock);
>+    if ( sock->closePending ) {
>+       PR_Unlock(sock->lock);
>+       JSSL_throwSSLSocketException(env, 
>+                "Accept operation failed socket is closing");
>+       goto finish;
>+    }

Please add ":", "--", or "because" between "failed" and "socket"
in the error message for the exception.

2. In jssl.h

>     PRFilePrivate *jsockPriv;
>     PRLock *lock;  /* protects reader, writer, and accepter */
>     PRThread *reader;
>     PRThread *writer;
>     PRThread *accepter;
>+    PRBool closePending;

Please update the comment to say:
    protects reader, writer, accepter, and closePending
Attachment #262446 - Flags: review?(wtc) → review+
(Assignee)

Comment 6

11 years ago
keeping bug open. I still plan to add a test program to all.pl. 


Checking in SSLServerSocket.c;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLServerSocket.c,v  <--  SSLServerSocket.c
new revision: 1.19; previous revision: 1.18
done
Checking in SSLSocket.c;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLSocket.c,v  <--  SSLSocket.c
new revision: 1.25; previous revision: 1.24
done
Checking in common.c;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/common.c,v  <--  common.c
new revision: 1.28; previous revision: 1.27
done
Checking in javasock.c;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/javasock.c,v  <--  javasock.c
new revision: 1.5; previous revision: 1.4
done
Checking in jssl.h;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/jssl.h,v  <--  jssl.h
new revision: 1.16; previous revision: 1.15
done
(Assignee)

Comment 7

11 years ago
I am unable to complete the cleanup for the test program before 
tomorrows release of 4.2.5. Marking this bug closed and will check the test program in for the next release using bug 380065. 
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.