On Sun, 22 Nov 2020 18:27:56 GMT, Christoph Langer <clan...@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
