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
