RFR: 8256818: SSLSocket that is never bound or connected leaks socket resources [v3]

Xue-Lei Andrew Fan xuelei at openjdk.java.net
Wed Dec 2 18:03:59 UTC 2020


On Sun, 22 Nov 2020 18:27:56 GMT, Christoph Langer <clanger at openjdk.org> wrote:

>> There is a flaw in sun.security.ssl.SSLSocketImpl::close() which leads to leaking socket resources after JDK-8224829.
>> 
>> The close method calls duplexCloseOutput() and duplexCloseInput(). In case of an exception in any of these methods, the call to closeSocket() is bypassed, and the underlying Socket may not be closed.
>> 
>> This manifests in a real life leak after JDK-8224829 has introduced a call to getSoLinger() on the path of duplexCloseOutput -> closeNotify. If socket impl / OS socket hadn't been created yet it is done at that place. But then after duplexCloseOutput eventually fails with a SocketException since the socket wasn't connected, closing fails to call Socket::close().
>> 
>> This problem can be reproduced by this code:
>> 		        SSLSocket sslSocket = (SSLSocket)SSLSocketFactory.getDefault().createSocket();
>> 		        sslSocket.getSSLParameters();
>> 		        sslSocket.close();
>> 
>> This is what happens when SSLContext.getDefault().getDefaultSSLParameters() is called, with close() being eventually called by the finalizer.
>> 
>> I'll open this PR as draft for now to start discussion. I'll create a testcase to reproduce the issue and add it soon.
>> 
>> I propose to modify the close method such that duplexClose is only done on a connected/bound socket. Maybe it even suffices to only do it when connected.
>> 
>> Secondly, I'm proposing to improve exception handling a bit. So in case there's an IOException on the path of duplexClose, it is caught and logged. But the real close moves to the finally block since it should be done unconditionally.
>
> Christoph Langer has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Small test improvement

test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java line 57:

> 55:         if ((fds_end - fds_start) > (NUM_TEST_SOCK / 10)) {
> 56:             throw new RuntimeException("Too many open file descriptors. Looks leaky.");
> 57:         }

This test case may be not reliable if there are some other test cases or applications running at the same time.  It's a good manual test, but might be not suitable for OpenJDK automation regression test if it could be impacted.

test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java line 37:

> 35:  *          will not leave leaking socket file descriptors
> 36:  * @library /test/lib
> 37:  * @run main/othervm SSLSocketLeak

See bellow comment, I may suggest to have it as a manual test case if you agree the test case could be impacted.
@run main/manual SSLSocketLeak

-------------

PR: https://git.openjdk.java.net/jdk/pull/1363



More information about the security-dev mailing list