8256818: SSLSocket that is never bound or connected leaks socket resources #1363

RealCLanger · 2020-11-21T08:32:17Z

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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8256818: SSLSocket that is never bound or connected leaks socket resources

Reviewers

Xue-Lei Andrew Fan (@XueleiFan - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1363/head:pull/1363
$ git checkout pull/1363

…urces

bridgekeeper · 2020-11-21T08:32:22Z

👋 Welcome back clanger! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2020-11-21T08:33:24Z

@RealCLanger The following label will be automatically applied to this pull request:

security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

RealCLanger · 2020-11-21T23:21:15Z

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.

mlbridge · 2020-11-21T23:24:45Z

Webrevs

RealCLanger · 2020-11-27T15:46:08Z

Ping... Any takers? comments? reviews?

XueleiFan · 2020-11-27T16:15:10Z

I will have a look at this update.

XueleiFan · 2020-12-02T17:57:04Z

test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java

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


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.

XueleiFan · 2020-12-02T18:01:04Z

test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java

+ * @summary Test that creating and closing SSL Sockets without bind/connect
+ *          will not leave leaking socket file descriptors
+ * @library /test/lib
+ * @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

Hm, I think it's fine as it is. Running it in othervm will make sure the test runs in its own vm (see http://openjdk.java.net/jtreg/command-help.html). So within the VM process there should not be any interference by other workload. And we check open files before testing and afterwards, and allow for some margin.

The test has been running in our test setup for several days now, so I think it should be ok. And if worst comes to worse, and we see test noise, we might change the test to manual later on.

Sounds good to me. Thanks!

openjdk · 2020-12-02T18:52:02Z

@RealCLanger This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8256818: SSLSocket that is never bound or connected leaks socket resources

Reviewed-by: xuelei

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 182 new commits pushed to the master branch:

692b273: 8257189: Handle concurrent updates of MH.form better
6704266: 8257565: epsilonBarrierSet.hpp should not include barrierSetAssembler
0b8c780: 8256256: UL should not use heap allocation for output string
2508bc7: 8257140: Crash in JvmtiTagMap::flush_object_free_events()
cfb50a9: 8253916: ResourceExhausted/resexhausted001 crashes on Linux-x64
287b829: 8254877: GCLogPrecious::_lock rank constrains what locks you are allowed to have when crashing
1fd0ea7: 8256382: Use try_lock for hs_err EventLog printing
bff68f1: 8257533: legacy-jre-image includes jpackage and jlink tools
9a60413: 8248736: [aarch64] runtime/signal/TestSigpoll.java failed "fatal error: not an ldr (literal) instruction."
e7ca0c4: 8257224: JDK-8251549 didn't update building.html
... and 172 more: https://git.openjdk.java.net/jdk/compare/9a19eb6918e1f766ccf1b1671ea1161a76fee571...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

RealCLanger · 2020-12-02T19:22:35Z

/integrate

openjdk · 2020-12-02T19:23:50Z

@RealCLanger Since your change was applied there have been 182 commits pushed to the master branch:

692b273: 8257189: Handle concurrent updates of MH.form better
6704266: 8257565: epsilonBarrierSet.hpp should not include barrierSetAssembler
0b8c780: 8256256: UL should not use heap allocation for output string
2508bc7: 8257140: Crash in JvmtiTagMap::flush_object_free_events()
cfb50a9: 8253916: ResourceExhausted/resexhausted001 crashes on Linux-x64
287b829: 8254877: GCLogPrecious::_lock rank constrains what locks you are allowed to have when crashing
1fd0ea7: 8256382: Use try_lock for hs_err EventLog printing
bff68f1: 8257533: legacy-jre-image includes jpackage and jlink tools
9a60413: 8248736: [aarch64] runtime/signal/TestSigpoll.java failed "fatal error: not an ldr (literal) instruction."
e7ca0c4: 8257224: JDK-8251549 didn't update building.html
... and 172 more: https://git.openjdk.java.net/jdk/compare/9a19eb6918e1f766ccf1b1671ea1161a76fee571...master

Your commit was automatically rebased without conflicts.

Pushed as commit 93b6ab5.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8256818: SSLSocket that is never bound or connected leaks socket reso…

878eea1

…urces

RealCLanger marked this pull request as draft November 21, 2020 08:32

openjdk bot added the security security-dev@openjdk.org label Nov 21, 2020

RealCLanger added 2 commits November 21, 2020 23:27

Only duplexClose when socket isConnected()

c5252bf

Add testcase for socket leak

c1663d9

openjdk bot added the core-libs core-libs-dev@openjdk.org label Nov 21, 2020

RealCLanger marked this pull request as ready for review November 21, 2020 23:21

openjdk bot added the rfr Pull request is ready for review label Nov 21, 2020

RealCLanger added 2 commits November 22, 2020 11:41

Simplify the test

34a1a21

Small test improvement

e280436

XueleiFan reviewed Dec 2, 2020

View reviewed changes

XueleiFan approved these changes Dec 2, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 2, 2020

openjdk bot closed this Dec 2, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 2, 2020

RealCLanger deleted the sslcontextstuff branch December 2, 2020 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8256818: SSLSocket that is never bound or connected leaks socket resources #1363

8256818: SSLSocket that is never bound or connected leaks socket resources #1363

RealCLanger commented Nov 21, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 21, 2020

openjdk bot commented Nov 21, 2020

RealCLanger commented Nov 21, 2020

mlbridge bot commented Nov 21, 2020 •

edited

Loading

RealCLanger commented Nov 27, 2020

XueleiFan commented Nov 27, 2020 •

edited

Loading

XueleiFan Dec 2, 2020

XueleiFan Dec 2, 2020

RealCLanger Dec 2, 2020

XueleiFan Dec 2, 2020

openjdk bot commented Dec 2, 2020

RealCLanger commented Dec 2, 2020

openjdk bot commented Dec 2, 2020

8256818: SSLSocket that is never bound or connected leaks socket resources #1363

8256818: SSLSocket that is never bound or connected leaks socket resources #1363

Conversation

RealCLanger commented Nov 21, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Nov 21, 2020

openjdk bot commented Nov 21, 2020

RealCLanger commented Nov 21, 2020

mlbridge bot commented Nov 21, 2020 • edited Loading

Webrevs

RealCLanger commented Nov 27, 2020

XueleiFan commented Nov 27, 2020 • edited Loading

XueleiFan Dec 2, 2020

Choose a reason for hiding this comment

XueleiFan Dec 2, 2020

Choose a reason for hiding this comment

RealCLanger Dec 2, 2020

Choose a reason for hiding this comment

XueleiFan Dec 2, 2020

Choose a reason for hiding this comment

openjdk bot commented Dec 2, 2020

RealCLanger commented Dec 2, 2020

openjdk bot commented Dec 2, 2020

RealCLanger commented Nov 21, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 21, 2020 •

edited

Loading

XueleiFan commented Nov 27, 2020 •

edited

Loading