282732 - Socket.close needs to interrupt threads blocked in I/O

Status: RESOLVED FIXED

(JSS Graveyard :: Library, defect)

Description

[glen beasley]

request to implement JSS SSLSocket.close so that it has the same behavior as the
JDK Socket.Close in the sense that any thread currently blocked in an I/O
operation upon this socket will throw a SocketException. 

http://java.sun.com/j2se/1.5.0/docs/api/java/net/Socket.html#close()

Comment 1

[glen beasley]

Attachment: Test Case to demostrate separate IO theads handling close

Comment 2

[glen beasley]

Attachment: read/write/close thread protection

The first call to SSLSocket.close or SSLServerSocket.close will close the
socket and release all native resources. Any further calls to  SSLSocket.close
or SSLServerSocket.close will return. The finalize method  will call
SSLSocket.close or SSLServerSocket.close incase the user did not. 

If a separate thread is performing a read or a write on the socket the socket
will throw an I/O exception, and exit gracefully. SocketBase.close is no longer
synchronized because SSLSocket.close and SSLServerSocket.close methods have
their own synchronization. 

The Sun JES Portal server team did week long testing of this fix for both the
Portal server and Netlet on the SUN_SECURITY_3_3_BRANCH. 

We still need a proper fix for NSPR on windows
https://bugzilla.mozilla.org/show_bug.cgi?id=282496 passing in PR_SHUTDOWN_BOTH
ends up sending an FIN/EOF to the other end, which will close the socket and
unblock the read operation.

Comment 3

[Wan-Teh Chang]

Comment on attachment 178193 [details] [diff] [review]
read/write/close thread protection

I recommend doing the following.

In the JSSL_SocketData structure or the PRFilePrivate
structure its jsockPriv member points to, add two
PRThread * members:

    PRThread *reader;
    PRThread *writer;

Add native methods to set the reader or writer member
to the current thread (the return value of PR_GetCurrentThread())
or to NULL.

The SSLSocket.read() method would set the reader member
to the current thread before calling socketRead(), and
set the reader member back to NULL before returning or
throwing an exception.	Similarly for the SSLSocket.write()
method.

Then, rename Java_org_mozilla_jss_ssl_SSLSocket_shutdownNativeLow
as Java_org_mozilla_jss_ssl_SSLSocket_unblockReaderWriter (because
unblocking reader and writer is what that method is really trying
to do), and instead of or in addition to calling PR_Shutdown, add
these PR_Interrupt calls:

    if (sock->reader) {
	PR_Interrupt(sock->reader);
    }
    if (sock->writer) {
	PR_Interrupt(sock->writer);
    }

This way you don't need any NSPR change related to PR_Shutdown.

Comment 4

[glen beasley]

Attachment: read/write/close thread protection

the main update to this patch is in the 
the Java_org_mozilla_jss_ssl_SSLSocket_abortReadWrite method 
I added comments in the method stating 

    * The java layer prevents I/O once closed has been 
    * called but if an I/O op was in progress then 
    * abort the I/O operation.
    * We could use PR_Interrupt or PR_Shutdown to abort the I/O
    * PR_Shutdown is more responsive on unix but PR_Interrupt
    * is required on windows to unblock the I/O. On 
    * WINNT PR_Interrupt does not work see 282496, so 
    * we rely on PR_Shutdown(fd, PR_SHUTDOWN_SEND) to send an
    * EOF to the other end to kill the socket, which as long as 
    * the other end ends kills the socket, the reader will then 
    * unblock.
    */ 
    /* when bug 282496 is fixed this code needs to be updated */
    /* to use PR_Interrupt on WINNT for the reader and writer */

Comment 5

[glen beasley]

Attachment: sorry had some debug pks11 that should not of been in last patch

Comment 6

[glen beasley]

note this bug also address PR_PENDING_INTERRUPT_ERROR
https://bugzilla.mozilla.org/show_bug.cgi?id=206332
It still does not handle the PR_IO_PENDING_ERROR case
https://bugzilla.mozilla.org/show_bug.cgi?id=206333

Comment 7

[glen beasley]

*** Bug 278434 has been marked as a duplicate of this bug. ***

Comment 8

[Wan-Teh Chang]

Glen, I am confused by your comment 4 about
WINNT PR_Interrupt and bug 282496.  Bug 282496
doesn't mention PR_Interrupt at all!

Comment 9

[Wan-Teh Chang]

Comment on attachment 178877 [details] [diff] [review]
sorry had some debug pks11 that should not of been in last patch 

Glen,

Sorry about the long review comments below.

1. In org/mozilla/jss/ssl/SSLServerSocket.java, I
think we also need a native abortAccept method so
that closing an SSL server socket will cause any
pending accept method calls to throw an exception,
right?

>     public Socket accept() throws IOException {
>+        if (isClosed) { /* abort accept if socket closed */
>+             throw new 
>+             IOException("SSLServerSocket has been closed, and cannot be reused."); 
>+        }

I think the testing and setting of the isClosed
class member needs to be synchronized.	Otherwise
it is not thread safe.	Since you already made
the close method (which sets isClosed) sycnchronized,
we just need to put the above code in a synchronized
block:
	  synchronized (this) {
	      if (isClosed) { /* abort accept if socket closed */
		  throw new 
		  IOException("SSLServerSocket has been closed, and cannot be
reused."); 
	      }
	  }

>+    protected void finalize() throws Throwable {
>+        this.close(); /* in Case user never called closed */
>+    }

"Case" should not be capitalized.

>+    public synchronized void close() throws IOException {
>+        isClosed=true;
>+        if( sockProxy != null ) { /* only call base.close once */
>+            sockProxy = null;
>+            base.close();
>+        }
>     }

Why does the whole close method need to be synchronized?

Do we really need to allow the close method to be called
multiple times?

It seems that isClosed is true if and only if sockProxy is
null, so the the body of the close method can be like this:

	  if( !isClosed ) { /* only call base.close once */
	      isClosed = true;
	      sockProxy = null;
	      base.close();
	  }

2. org/mozilla/jss/ssl/SSLSocket.c

>+#include <prthread.h>

This is not necessary because this file already includes
<nspr.h>, which includes all NSPR headers.

>+    /* set the current thread doing the read*/
>+    sock->reader = PR_GetCurrentThread();

Please add
      PR_ASSERT(sock->reader == NULL);
before setting sock->reader to PR_GetCurrentThread().

>@@ -717,28 +720,33 @@
>         } else {
>             /* some error, but is it recoverable? */
>             PRErrorCode err = PR_GetError();
> 
> 
>-            if( err == PR_PENDING_INTERRUPT_ERROR ||
>-                err == PR_IO_PENDING_ERROR )
>+            if( err == PR_IO_PENDING_ERROR )
>             {
>                 /* just try again */
>             } else {
>-#ifdef WINNT
>+		if (err == PR_PENDING_INTERRUPT_ERROR ) {
>+                    JSSL_throwSSLSocketException(env, 
>+				"Read operation interrupted");
>+                    goto finish;
>+		}

You need to call PR_NT_CancelIo for WINNT when you receive
the PR_PENDING_INTERRUPT_ERROR.  This is similar to
PR_IO_TIMEOUT_ERROR.

Perhaps this is why you had strange problems after calling
PR_Interrupt on WINNT?

>                 if (err == PR_IO_TIMEOUT_ERROR ) {
>+#ifdef WINNT
>+
>                 /*
>                  * if timeout was set, and the PR_Accept() timed out,
>                  * then cancel the I/O on the port, otherwise PR_Accept()
>                  * will always return PR_IO_PENDING_ERROR on subsequent
>                  * calls
>                  */
>                     PR_NT_CancelIo(sock->fd);
>+#endif 

In the comment, change "PR_Accept" to "PR_Recv" (two occurrences).

>+    /*Set the current thread doing the write */
>+    sock->writer = PR_GetCurrentThread();

Add
      PR_ASSERT(sock->writer == NULL);

>+            if(err == PR_IO_PENDING_ERROR)
>             {
>                 /* just try again */
>-            } else if( err == PR_IO_TIMEOUT_ERROR ) {
>+            } else  if (err == PR_PENDING_INTERRUPT_ERROR ) {
>+                    JSSL_throwSSLSocketException(env, "Write operation interrupted");
>+                    goto finish;

Need to call PR_NT_CancelIo for WINNT.

>+		} else if( err == PR_IO_TIMEOUT_ERROR ) {
> #ifdef WINNT
>                 /*
>                  * if timeout was set, and the PR_Accept() timed out,
>                  * then cancel the I/O on the port, otherwise PR_Accept()
>                  * will always return PR_IO_PENDING_ERROR on subsequent

Need to change "PR_Accept" to "PR_Send" in the comment.

> JNIEXPORT void JNICALL
>+Java_org_mozilla_jss_ssl_SSLSocket_abortReadWrite(
>+    JNIEnv *env, jobject self)
>+{
...
>+    /*
>+    * The java layer prevents I/O once closed has been 
>+    * called but if an I/O op is in progress then 
>+    * abort the I/O operation.
>+    * We could use PR_Interrupt or PR_Shutdown to abort the I/O
>+    * PR_Shutdown is more responsive on unix but PR_Interrupt
>+    * is required on windows to unblock the I/O. On 
>+    * WINNT PR_Interrupt does not work see 282496, so 
>+    * we rely on PR_Shutdown(fd, PR_SHUTDOWN_SEND) to send an
>+    * EOF to the other end to kill the socket, which as long as 
>+    * the other end ends kills the socket, the reader will then 
>+    * unblock.
>+    */ 
>+    /* when bug 282496 is fixed this code needs to be updated */
>+    /* to use PR_Interrupt on WINNT for the reader and writer */

This comment needs work.  It is self-contradictory.  It first
says "PR_Interrupt is required on windows" and then says
"On WINNT PR_Interrupt does not work ... so we rely on
PR_Shutdown..."

>+    if (sock->reader) {
>+	PR_Shutdown(fd, PR_SHUTDOWN_RCV); 
>+#ifdef WINNT 
>+	PR_Shutdown(fd, PR_SHUTDOWN_SEND);
>+#endif
>+    }
>+
>+    if (sock->writer) {
>+	PR_Shutdown(fd, PR_SHUTDOWN_SEND);
>+    }

My original suggestion was to do the following:

>+    if (sock->reader) {
>+	PR_Interrupt(sock->reader);
>+    }
>+
>+    if (sock->writer) {
>+	PR_Interrupt(sock->writer);
>+    }

We should find out why PR_Interrupt doesn't work.
We should use PR_Shutdown only on platforms where
PR_Interrupt doesn't work.

3. org/mozilla/jss/ssl/SSLSocket.java

There are some thread synchronization problems in
this file.

I suggest the following code for the close,
read, and write methods.

    public void close() throws IOException {
	// this ugly nesting of synchronized blocks can
	// be avoided at the cost of having separate
	// abortRead and abortWrite methods.
	synchronized (readLock) {
	    synchronized (writeLock) {
		/*if a read or write is occuring abort the I/O, which will
cause any
		 *further attempts to read/write to fail since "isClosed==true"
		 */
		if (ioRead || ioWrite) {
		    abortReadWrite();
		}
		while (ioWrite) {
		    writeLock.wait();
		}
	    }
	    while (ioRead) {
		readLock.wait();
	    }
	}
	synchronized (this) {
	    if	(!isClosed) {  /* only call base.close once
		isClosed = true;
		sockProxy = null;
		base.close();
	    }
	}
    }

    int read(byte[] b, int off, int len) throws IOException {
	synchronized (this) {
	    if (isClosed) { /* abort read if socket closed */
		throw new 
		IOException("Socket has been closed, and cannot be reused."); 
	    }
	}
	int iRet = 0;
	synchronized (readLock) {
	    ioRead=true;
	    try {
		iRet = socketRead(b, off, len, base.getTimeout()); 
	    } catch (IOException ioe) {
		ioRead=false;
		throw new 
		IOException("SocketException cannot read on socket");
	    }
	    ioRead=false;
	}
	return iRet;
    }

Similarly for the write method (omitted for brevity).

4. org/mozilla/jss/ssl/common.c

> JNIEXPORT void JNICALL
> Java_org_mozilla_jss_ssl_SocketProxy_releaseNativeResources
>     (JNIEnv *env, jobject this)
>+{  
>+   /* call to socket.close will destroy all Native resources  */
>+   /* if user does call close on the socket the finalize method will*/
> }

Why is this method a no-op?  Can we remove it altogether?  It
is very confusing to have a method that does nothing.

OK, that's it!

Comment 10

[Wan-Teh Chang]

Comment on attachment 178877 [details] [diff] [review]
sorry had some debug pks11 that should not of been in last patch 

Sorry, I forgot to mention one problem.  In org/mozilla/jss/ssl/common.c

>+   /* if user does call close on the socket the finalize method will*/

should that be "if the user does not call close..."?

Comment 11

[glen beasley]

Attachment: added comment for PR_Interrupt bug 282496

in reply to comment 8

I updated bug 282496 to mention PR_Interrupt,  
created a seperate bug for PR_Interrupt 288232, and 
changed abortReadWrite methods comments. 

Although 288232 or 282496 needs to be solved, I would like to check 
in this JSS bug so that our internal JES 4 products can use the fix
with the understanding that on WINNT we rely on the remote connection
closing the SSL socket first to unblock the read before closing the 
socket.

Java_org_mozilla_jss_ssl_SSLSocket_abortReadWrite(
    JNIEnv *env, jobject self)
{
    JSSL_SocketData *sock = NULL;
    PRStatus status;
    PRFileDesc *fd = NULL;

    if( JSSL_getSockData(env, self, &sock) != PR_SUCCESS) goto finish;

    /*find the lowest fileDescriptor */

    fd = sock->fd;
    if (fd == NULL) {
	JSS_throw(env, NULL_POINTER_EXCEPTION);
	goto finish;
    }
    while (fd->lower) {
	fd = fd->lower;
    }
    /*
    * The java layer prevents I/O once closed has been 
    * called but if an I/O op is in progress then 
    * abort the I/O operation.
    * We could use PR_Interrupt or PR_Shutdown to abort the I/O
    * PR_Shutdown is more responsive on unix but PR_Interrupt
    * is required on windows to unblock the I/O. On 
    * WINNT PR_Interrupt does not work see 288232 and 282496, so 
    * we rely on PR_Shutdown(fd, PR_SHUTDOWN_SEND) to send an
    * EOF to the other end to kill the socket, which as long as 
    * the other end ends kills the socket, the reader will then 
    * unblock.
    */ 
    /* when bug 288232 or 282496 is fixed this code needs to be updated */
    /* to use PR_Interrupt on WINNT for the reader and writer */
    if (sock->reader) {
	PR_Shutdown(fd, PR_SHUTDOWN_RCV); 
#ifdef WINNT 
	PR_Shutdown(fd, PR_SHUTDOWN_SEND);
#endif
    }

    if (sock->writer) {
	PR_Shutdown(fd, PR_SHUTDOWN_SEND);
    }
finish:
    EXCEPTION_CHECK(env, sock)
    return;
}

Comment 12

[Wan-Teh Chang]

Please ignore my suggested rewrite of the close,
read, and write methods for now.  It is wrong.

Comment 13

[glen beasley]

Attachment: calling PR_NT_CancelIo in socketRead and socketWrtie

replying to comment 12

Wan-teh your rewrite does work. By calling PR_NT_CancelIo in SocketRead and 
SocketWrite when err == PR_PENDING_INTERRUPT_ERROR the solution works
correctly.
I tested several times, with no issue.
Note socket.close will wait until ioRead and ioWrite are false because the 
readLock and writeLock locks will not become free until the threads exit the
socket.read and socket.write. 
After you review this fix, I will close bug 288232

Comment 14

[glen beasley]

Attachment: Test program for ublocking SSLServerSocket.accept when closing

Test program puts a thread blocked on accept, and has the main 
thread call close to unblock the accept thread and the close 
the SSLServerSocket.

Comment 15

[glen beasley]

Attachment: SSLServerSocket and SSLSocket protection for close methods

Reply to comment 9

1) I added support for abortAccept, I also attached a test program for 
this case. 

1a) the isClosed flag does not need to be synchronized because the close 
method now has acceptLock and the close method itself is not synchronized.
(orginally I was going to add proper protection for SSLServerSocket after
SSLSocket was completed but now this bug addresses both). 

2) done

3 I tested the thread synchronization, and reviewed the code 
with Nelson several times. I would like to keep the nested blocks in 
SSLSocket.close. Once a thread goes into the close method the private boolean 
isClosed is set to true, (it is never set to false afterwards.) In the close
method when a reader or writer needs to be aborted, after they are aborted the
locks will be free for the close method to acquire. Any attempts by other
threads to read/write will be aborted since the isClosed flag is set to true
and the threads will return from the read/write methods instead of being
allowed to acquire a lock again.

4) done

Comment 16

[Wan-Teh Chang]

Attachment: wtc's patch

Glen,

I fixed the thread synchronization problems in your
patch.	Please look at my comments "Locking strategy"
in SSLServerSocket.java and SSLSocket.java, and
my comment in jssl.h next to the "PRLock *lock;"
member of struct JSSL_SocketData.

I also some other code cleanup and bug fixes (the
bugs are not all in your patch; some are preexisting).

Now that I understand the code better, I'm not sure
I like how you fixed the resource leaks.  The reason
is that now SocketProxy.releaseNativeResources has an
empty body (I found that this method must be defined
because it is an abstract method in the base class).

So now we allow user to call close more than once.
Should we?  Can a standard Java socket be closed
more than once?

Note that I got rid of the for loops around PR_Accept,
PR_Recv, and PR_Send because they are not necessary.
If you call PR_NT_CancelIo properly, you will never
get PR_IO_PENDING_ERROR.  And even if you get
PR_IO_PENDING_ERROR for some reason, you should not
loop back to retry the I/O call.

I got rid of the necessary code in abortAccept and
abortReadWrite to find the lowest file descriptor.

I have one question about your code, which I marked
with a "XXX" comment.  Please take a look at it.

My version of your patch passed the two tests of yours.

Comment 17

[glen beasley]

Attachment: checkin of patch

Wan-teh and I had in-person code review meeting. 
checkins:
/cvsroot/mozilla/security/jss/lib/jss.def,v  <--  jss.def
new revision: 1.28; previous revision: 1.27
done
 cvs commit -m "282732 read/write/accept/close thread protection"
cvs commit: Examining .
Enter passphrase for key '/home/gb134726/.ssh/id_dsa': 
? .nbattrs
Checking in SSLServerSocket.c;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLServerSocket.c,v  <-- 
SSLServerSocket.c
new revision: 1.15; previous revision: 1.14
done
Checking in SSLServerSocket.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLServerSocket.java,v  <-- 
SSLServerSocket.java
new revision: 1.18; previous revision: 1.17
done
Checking in SSLSocket.c;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLSocket.c,v  <-- 
SSLSocket.c
new revision: 1.22; previous revision: 1.21
done
Checking in SSLSocket.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLSocket.java,v  <-- 
SSLSocket.java
new revision: 1.20; previous revision: 1.19
done
Checking in SocketBase.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SocketBase.java,v  <-- 
SocketBase.java
new revision: 1.12; previous revision: 1.11
done
Checking in common.c;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/common.c,v  <--  common.c
new revision: 1.22; previous revision: 1.21
done
Checking in jssl.h;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/jssl.h,v  <--  jssl.h
new revision: 1.15; previous revision: 1.14
done

Comment 18

[glen beasley]

This bug also fixed bug 206332 and bug 20633

Comment 19

[glen beasley]

meant this bug fixed bug 206333 not 20633