304195 - toString() call in SSLSocket.java does not check for exceptions

Status: RESOLVED FIXED

(JSS Graveyard :: Library, defect)

Description

[Sandeep Konchady]

There has been instances when a client calls SSLSocket.toString() when the
socket has already been closed.  This generates exception.  In order for us to
make this safe, we need to handle the exceptions ourselves within the toString()
method.

Comment 1

[Sandeep Konchady]

Attachment: Modified toString() method that checks for exception

The toString() method now checks for exception when calling getInetAddress(),
getLocalAddress(), getPort(), and getLocalPort().  When an exception is caught
the return string will be the exception message.  If not the returned string
will be the InetAddress and Port information about the socket.

Comment 2

[glen beasley]

Comment on attachment 192258 [details] [diff] [review]
Modified toString() method that checks for exception

I would change the comment to something like this:
* Returns the addresses and ports of this socket
* or an error message if 
the socket is not valid state.
An error message can happen even if the socket is not closed by the
application, but communication has ended on the other end, and the exception
back would be 
(-5978) Network file descriptor is not connected. 

so you might want to change the boolean socketClosed to exception OnSocket

Also, you should modify the toString to SSLServerSocket as well.

Comment 3

[Sandeep Konchady]

Attachment: Modified toString() methods in SSLSocket.java and SSLServerSocket.java to capture exceptions

As per Glen's suggestion modified the comments as well as the exception flag in
both SSLSocket.java and SSLServerSocket.java.

Comment 4

[Wan-Teh Chang]

Comment on attachment 192264 [details] [diff] [review]
Modified toString() methods in SSLSocket.java and SSLServerSocket.java to capture exceptions

Seems like the Java way is to put the try block around the
whole thing and do without the exceptionOnSocket boolean,
like this:

    /**
     * Returns the addresses and ports of this socket
      * or an error message if the socket is not in a valid state.
      */
    public String toString() {
	try {
	    StringBuffer buf = new StringBuffer();
	    InetAddress inetAddr = getInetAddress();
	    int localPort = getLocalPort();
	    buf.append("SSLServerSocket[addr=");
	    buf.append(inetAddr);
	    buf.append(",port=0,localport=");
	    buf.append(localPort);
	    buf.append("]");
	    return buf.toString();
	} catch (OutOfMemoryException e) {
	    throw e;
	} catch (Exception e) {
	    return "Exception caught in toString(): " + e.getMessage();
	}
    }

Comment 5

[Sandeep Konchady]

Attachment: Modified as per Wan-Teh's suggestion by putting the whole block under try/catch

Modified the source as per Wan-Teh's suggestion by putting the whole block in
toString() under try/catch.

Comment 6

[Wan-Teh Chang]

Comment on attachment 192303 [details] [diff] [review]
Modified as per Wan-Teh's suggestion by putting the whole block under try/catch

r=wtc.	Be sure to get Glen's review, too, because I am
not a Java expert.

Some comments:

1. So we can't possibly get OutOfMemoryException from either
the new operator of the StringBuffer.append method?

2. It seems that for SSLServerSocket there is no point printing
the useless "port=0" string.

3. In SSLSocket.toString, I noticed this change:

>-        buf.append("SSLSocket[addr=");
>-        buf.append(getInetAddress());
>-        buf.append(getLocalAddress());
>-        buf.append(",port=");

>+            buf.append("SSLSocket[addr=");
>+            buf.append(inetAddr);
>+            buf.append(",localaddr=");
>+            buf.append(localAddr);
>+            buf.append(",port=");

The original code prints inet address and local address
with nothing in between, whereas the new code inserts
",localaddr=" between the two, which seems better.

Comment 7

[glen beasley]

Comment on attachment 192303 [details] [diff] [review]
Modified as per Wan-Teh's suggestion by putting the whole block under try/catch

looks great.

Comment 8

[Sandeep Konchady]

Comment on attachment 192303 [details] [diff] [review]
Modified as per Wan-Teh's suggestion by putting the whole block under try/catch

Checking in SSLSocket.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLSocket.java,v  <-- 
SSLSocket.java
new revision: 1.7.8.2.2.4.2.10; previous revision: 1.7.8.2.2.4.2.9
done
Checking in SSLServerSocket.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLServerSocket.java,v  <-- 
SSLServerSocket.java
new revision: 1.5.4.1.2.3.2.5; previous revision: 1.5.4.1.2.3.2.4
done

Comment 9

[Sandeep Konchady]

Comment on attachment 192303 [details] [diff] [review]
Modified as per Wan-Teh's suggestion by putting the whole block under try/catch

Checking in SSLSocket.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLSocket.java,v  <-- 
SSLSocket.java
new revision: 1.21; previous revision: 1.20
done
Checking in SSLServerSocket.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLServerSocket.java,v  <-- 
SSLServerSocket.java
new revision: 1.19; previous revision: 1.18
done