On Sat, 21 Nov 2020 23:21:15 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.
>
> I changed the check for when to do duplexClose to only do it when socket 
> isConnected().
> 
> I also added a testcase which should work on all platforms. For windows I 
> borrowed some functionality introduced lately with test 
> java/lang/ProcessBuilder/checkHandles/CheckHandles.java which I moved to the 
> test library for that reason.
> 
> Now it's ready to review.

Ping... Any takers? comments? reviews?

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

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

Reply via email to