Here is a list of issues to be evaluated.

- Change
import com.sun.net.ssl.internal.ssl.CipherBox.*;
to
import com.sun.net.ssl.internal.ssl.CipherBox.CipherNULL;

- Delete the instance variable 'port' because it gets assigned (in
connect()) but is otherwise not used.

- Delete the following unused constructor
SSLSocketImpl(SSLContextImpl context, Socket sock, byte clientAuth)
throws IOException

- init() method has the following
try {
...
} catch (Exception e) {
    // can't happen
}

Should catch a specific exception and act appropriately or explain why
can't happen.

- Delete the following comment for enableByDefault(); not relevant any more.
    /*
     * XXX want better ways to determine if suite has a null cipher
     * or is anonymous ... this works for standard SSLv3 cipher suites,
     * but not for extensions. Prefer to have session options that
     * expose information more usefully -- e.g. what key exchange is
     * used, what cipher is used, what MAC, etc.
     */

- in clearPipeline(), the following code can be simplified and
catch (Throwable t) should be eliminated

} catch (SSLException e) {
if (connectionState != cs_CLOSED) {
try {
// make double sure that the connection
// is closed and we reported an error
try {
fatal(alert_handshake_failure, "");
} catch (Throwable t) {
}
throw e;

} catch (Exception x) { // always throws
// ... but throw the _original_ exception
throw e;
}
} else
throw e;
}
To perhaps:
} catch (SSLException e) {
if (connectionState != cs_CLOSED) {
// make double sure that the connection
// is closed and we reported an error
try {
fatal(alert_handshake_failure, "");
} catch (IOException t) {
// Ignore failure to send fatal alert
}
}
throw e; // rethrow the original exception
}

- Delete comment above isClosed() because it is no longer applicable

    // package private
    public boolean isClosed()
to
    public boolean isClosed()

- isClose() should use getConnectionState() instead of directly
accessing connectionState field due to synchronization issues

- recvAlert(): delete the following comment

// XXX log -- strange protocol behaviour


- getHost(): this method updates the 'host' instance field. Does it
need to be synchronized? It is used in ClientHandshaker.

- setUseClientMode():
try {
getHandshaker();
} catch (Exception e) {
e.printStackTrace();
}
* Replace Exception with SSLException
* Replace e.printStackTrace() with appropriate action
* In 'default' switch, should throw IllegalStateException instead of
doing nothing

- getSession(): delete // XXX comment

- setEnabledCipherSuites(): no need to declare throws IllegalArgumentException

- startHandshake():

* Delete the following comment

/*
* This is probably broken but will be sychronous for clients
* which just created a socket but haven't done IO yet.
*/
* Is use of connectionState OK? Use getConnectionState() instead for
synchronized access?

- filterEnabledCipherSuites:

* no need to declare throws IllegalArgumentException
* better name might be 'verifyEnabledCipherSuites' or 'checkEnabledCipherSuites'
* Uses CipherSpec.getSupportedCipherSuites(), which returns a clone.
Can a non-clone version be made available and used to avoid the cloning cost?
* Perhaps this method shouldn't be returning anything. The caller then should
clone array if verification passes. This way, cloning/copying is done
only upon successful verification.

In SSLSocketImpl.setEnabledCipherSuites and
SSLServerSocketImpl.setEnabledCipherSuites, change

enabledCipherSuites = filterEnabledCipherSuites(suites);
To
        checkEnabledCipherSuites(suites);
        enabledCipherSuites = (String[]) suites.clone();


- How come SSLServerSocketImpl.setEnabledCipherSuites() and
SSLServerSocketImpl.getEnabledCipherSuites() are not synchronized but
the SSLSocketImpl versions are synchronized?