Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl #2057

Closed
wants to merge 7 commits into from

Conversation

@cliveverghese
Copy link
Contributor

@cliveverghese cliveverghese commented Jan 13, 2021

Redo for 8237578: JDK-8214339 (SSLSocketImpl wraps SocketException) appears to not be fully fixed

This also fixes JDK-8259516: Alerts sent by peer may not be received correctly during TLS handshake


Progress

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

Issue

  • JDK-8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 13, 2021

👋 Welcome back cverghese! 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 openjdk bot added the rfr label Jan 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 13, 2021

@cliveverghese The following labels will be automatically applied to this pull request:

  • net
  • security

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 13, 2021

Webrevs

Copy link
Member

@dfuch dfuch left a comment

The changes to the HttpClient test look reasonable to me. I'll let the security libs experts comment on the rest of this change however.

best regards,
-- daniel

@cliveverghese
Copy link
Contributor Author

@cliveverghese cliveverghese commented Jan 13, 2021

/issue add JDK-8259516

@openjdk
Copy link

@openjdk openjdk bot commented Jan 13, 2021

@cliveverghese
Adding additional issue to issue list: 8259516: Alerts sent by peer may not be received correctly during TLS handshake.

//

/*
* @test
Copy link
Member

@rhalade rhalade Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @test
* @test
* @bug 8259662 8259516

Copy link
Contributor Author

@cliveverghese cliveverghese Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback. I have updated the change to include the bug ids.

@cliveverghese cliveverghese changed the title 8259662: SocketException should be passed through 8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl Jan 21, 2021
@cliveverghese cliveverghese force-pushed the 8259662 branch 2 times, most recently from 983842d to 72fef9b Jan 26, 2021
@XueleiFan
Copy link
Member

@XueleiFan XueleiFan commented Jan 27, 2021

For the CSR request, I updated the component to security-libs/javax.net.ssl, add 17 as one of the fix versions, and added myself as reviewer.

@cliveverghese
Copy link
Contributor Author

@cliveverghese cliveverghese commented Jan 27, 2021

Thank you. I will change the status of the CSR to proposed.

if (cause instanceof SocketException) {
if (!conContext.isInboundClosed()) {
try {
decode(null);
Copy link
Member

@XueleiFan XueleiFan Jan 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One decode may be not sufficient if there are multiple pending handshake messages record, or the connection has been established and application data transportation is in progress. I'm not sure if an additional decode() here really solve the problem as described in JDK-8259516.

I did not follow the issue of JDK-8259516. Why an SSLException is desired but not SocketException, as if the socket has been closed?

Copy link
Contributor Author

@cliveverghese cliveverghese Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed issues for JDK-8259516.

Copy link
Member

@XueleiFan XueleiFan left a comment

Please make sure all tier1 and tier2 tests passed on all supported platforms. Except the failed I commented in the update, javax/net/ssl/SSLSession/TestEnabledProtocols.java is also failed with a similar stack on Windows. Maybe, the pull request command "/test tier1" and "/test tier2" could help speed up the testing.

try {
sslIS.read();
sslOS.write('A');
sslOS.flush();
} catch (SSLHandshakeException se) {
// Ignore exception as this is expected.
}
}
Copy link
Member

@XueleiFan XueleiFan Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test cannot pass tier2 test on Windows.

java.net.SocketException: An established connection was aborted by the software in your host machine
	at java.base/sun.nio.ch.NioSocketImpl.implWrite(NioSocketImpl.java:420)
	at java.base/sun.nio.ch.NioSocketImpl.write(NioSocketImpl.java:440)
	at java.base/sun.nio.ch.NioSocketImpl$2.write(NioSocketImpl.java:826)
	at java.base/java.net.Socket$SocketOutputStream.write(Socket.java:1045)
	at java.base/sun.security.ssl.SSLSocketOutputRecord.flush(SSLSocketOutputRecord.java:266)
	at java.base/sun.security.ssl.HandshakeOutStream.flush(HandshakeOutStream.java:89)
	at java.base/sun.security.ssl.CertificateRequest$T12CertificateRequestProducer.produce(CertificateRequest.java:629)
	at java.base/sun.security.ssl.SSLHandshake.produce(SSLHandshake.java:440)
	at java.base/sun.security.ssl.ClientHello$T12ClientHelloConsumer.consume(ClientHello.java:1120)
	at java.base/sun.security.ssl.ClientHello$ClientHelloConsumer.onClientHello(ClientHello.java:853)
	at java.base/sun.security.ssl.ClientHello$ClientHelloConsumer.consume(ClientHello.java:812)
	at java.base/sun.security.ssl.SSLHandshake.consume(SSLHandshake.java:396)
	at java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:480)
	at java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:458)
	at java.base/sun.security.ssl.TransportContext.dispatch(TransportContext.java:199)
	at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:172)
	at java.base/sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:1501)
	at java.base/sun.security.ssl.SSLSocketImpl.readHandshakeRecord(SSLSocketImpl.java:1415)
	at java.base/sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:450)
	at java.base/sun.security.ssl.SSLSocketImpl.ensureNegotiated(SSLSocketImpl.java:915)
	at java.base/sun.security.ssl.SSLSocketImpl$AppInputStream.read(SSLSocketImpl.java:1006)
	at java.base/sun.security.ssl.SSLSocketImpl$AppInputStream.read(SSLSocketImpl.java:966)
	at ShouldThrowSSLExceptionWhenPeerClosesSocket.runServerApplication(ShouldThrowSSLExceptionWhenPeerClosesSocket.java:131)
	at SSLSocketTemplate.doServerSide(SSLSocketTemplate.java:280)
	at SSLSocketTemplate.startServer(SSLSocketTemplate.java:584)
	at SSLSocketTemplate.bootup(SSLSocketTemplate.java:498)
	at SSLSocketTemplate.run(SSLSocketTemplate.java:83)
	at ShouldThrowSSLExceptionWhenPeerClosesSocket.main(ShouldThrowSSLExceptionWhenPeerClosesSocket.java:292)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
	at java.base/java.lang.Thread.run(Thread.java:831)
	Suppressed: java.net.SocketException: An established connection was aborted by the software in your host machine
		at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:325)
		at java.base/sun.nio.ch.NioSocketImpl.read(NioSocketImpl.java:350)
		at java.base/sun.nio.ch.NioSocketImpl$1.read(NioSocketImpl.java:803)
		at java.base/java.net.Socket$SocketInputStream.read(Socket.java:976)
		at java.base/sun.security.ssl.SSLSocketInputRecord.read(SSLSocketInputRecord.java:478)
		at java.base/sun.security.ssl.SSLSocketInputRecord.readHeader(SSLSocketInputRecord.java:472)
		at java.base/sun.security.ssl.SSLSocketInputRecord.decode(SSLSocketInputRecord.java:160)
		at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:111)
		at java.base/sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:1501)
		at java.base/sun.security.ssl.SSLSocketImpl.handleException(SSLSocketImpl.java:1696)
		at java.base/sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:460)
		... 15 more

os.flush();
} catch (SSLProtocolException | SSLHandshakeException sslhe) {
throw sslhe;
} catch (SSLException ssle) {
} catch (SocketException se) {
// the expected exception, ignore it
System.err.println("server exception: " + ssle);
System.err.println("server exception: " + se);
Copy link
Member

@XueleiFan XueleiFan Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test failed with on Linux/Windows/MacOSX:

javax.net.ssl.SSLHandshakeException: Insufficient buffer remaining for AEAD cipher fragment (2). Needs to be more than tag size (16)
	at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:131)
	at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:369)
	at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:312)
	at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:307)
	at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:134)
	at java.base/sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:1501)
	at java.base/sun.security.ssl.SSLSocketImpl.handleException(SSLSocketImpl.java:1696)
	at java.base/sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:460)
	at java.base/sun.security.ssl.SSLSocketImpl.ensureNegotiated(SSLSocketImpl.java:915)
	at java.base/sun.security.ssl.SSLSocketImpl$AppOutputStream.write(SSLSocketImpl.java:1285)
	at java.base/sun.nio.cs.StreamEncoder.writeBytes(StreamEncoder.java:242)
	at java.base/sun.nio.cs.StreamEncoder.implFlushBuffer(StreamEncoder.java:321)
	at java.base/sun.nio.cs.StreamEncoder.implFlush(StreamEncoder.java:325)
	at java.base/sun.nio.cs.StreamEncoder.flush(StreamEncoder.java:159)
	at java.base/java.io.OutputStreamWriter.flush(OutputStreamWriter.java:248)
	at java.base/java.io.BufferedWriter.flush(BufferedWriter.java:257)
	at SocketExceptionForSocketIssues.test(SocketExceptionForSocketIssues.java:79)
	at SocketExceptionForSocketIssues.main(SocketExceptionForSocketIssues.java:45)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
	at java.base/java.lang.Thread.run(Thread.java:831)
	Suppressed: java.net.SocketException: Broken pipe
		at java.base/sun.nio.ch.NioSocketImpl.implWrite(NioSocketImpl.java:420)
		at java.base/sun.nio.ch.NioSocketImpl.write(NioSocketImpl.java:440)
		at java.base/sun.nio.ch.NioSocketImpl$2.write(NioSocketImpl.java:826)
		at java.base/java.net.Socket$SocketOutputStream.write(Socket.java:1045)
		at java.base/sun.security.ssl.SSLSocketOutputRecord.encodeAlert(SSLSocketOutputRecord.java:81)
		at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:400)
		... 22 more

@cliveverghese
Copy link
Contributor Author

@cliveverghese cliveverghese commented Jan 29, 2021

/issue remove JDK-8259516

@openjdk
Copy link

@openjdk openjdk bot commented Jan 29, 2021

@cliveverghese
Removing additional issue from issue list: 8259516.

@cliveverghese
Copy link
Contributor Author

@cliveverghese cliveverghese commented Jan 29, 2021

/test tier1

@openjdk
Copy link

@openjdk openjdk bot commented Jan 29, 2021

@cliveverghese you need to get approval to run the tests in tier1 for commits up until 573406d

@openjdk openjdk bot added the test-request label Jan 29, 2021
@cliveverghese
Copy link
Contributor Author

@cliveverghese cliveverghese commented Jan 29, 2021

/test tier2

@openjdk
Copy link

@openjdk openjdk bot commented Jan 29, 2021

@cliveverghese the test group tier2 does not exist

@cliveverghese
Copy link
Contributor Author

@cliveverghese cliveverghese commented Feb 2, 2021

Hi,

Thank you for the feedback.

I have verified that tier1 and tier2 tests pass on Windows, MacOS and Linux.

@XueleiFan
Copy link
Member

@XueleiFan XueleiFan commented Feb 2, 2021

Hi,

Thank you for the feedback.

I have verified that tier1 and tier2 tests pass on Windows, MacOS and Linux.

Thank you! I will address the closed test failures. I will post you when the update is ready.

@XueleiFan
Copy link
Member

@XueleiFan XueleiFan commented Feb 3, 2021

/csr needed

@openjdk openjdk bot added the csr label Feb 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 3, 2021

@XueleiFan this pull request will not be integrated until the CSR request JDK-8259847 for issue JDK-8259662 has been approved.

Copy link
Member

@XueleiFan XueleiFan left a comment

It looks good to me now. I will take care of the closed test update. Please finalize the CSR request before integration.

@openjdk openjdk bot removed the csr label Feb 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 19, 2021

@cliveverghese this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8259662
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk
Copy link

@openjdk openjdk bot commented Feb 19, 2021

@cliveverghese 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:

8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl

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 1 new commit pushed to the master branch:

  • b10376b: 8261938: ASN1Formatter.annotate should not return in the finally block

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dfuch, @rhalade, @XueleiFan) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added ready and removed merge-conflict labels Feb 19, 2021
@cliveverghese
Copy link
Contributor Author

@cliveverghese cliveverghese commented Feb 20, 2021

/integrate

@openjdk openjdk bot added the sponsor label Feb 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 20, 2021

@cliveverghese
Your change (at version a746864) is now ready to be sponsored by a Committer.

@XueleiFan
Copy link
Member

@XueleiFan XueleiFan commented Feb 22, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Feb 22, 2021

@XueleiFan @cliveverghese Since your change was applied there have been 21 commits pushed to the master branch:

  • cf0019d: 8259942: Enable customizations in CompileJavaModules.gmk and Main.gmk
  • a6a7e43: 8242032: G1 region remembered sets may contain non-coarse level PRTs for already coarsened regions
  • d7eebda: 8261391: ZGC crash - SEGV in RevokeOneBias::do_thread
  • 5b7b18c: 8259800: timeout in tck test testForkJoin(ForkJoinPool8Test)
  • 419717d: 8261803: Remove unused TaskTerminator in g1 full gc ref proc executor
  • 011f5a5: 8261799: Remove unnecessary cast in psParallelCompact.hpp
  • e9d7c07: 8248318: Remove superfluous use of boxing in ObjectStreamClass
  • 6b7575b: 8228748: Remove GCLocker::_doing_gc
  • c20fb5d: 8261448: Preserve GC stack watermark across safepoints in StackWalk
  • 26c1db9: 8254239: G1ConcurrentMark.hpp unnecessarily disables MSVC++ warning 4522.
  • ... and 11 more: https://git.openjdk.java.net/jdk/compare/977a21add8d275dae802a769e6c63675a8ec3ea9...master

Your commit was automatically rebased without conflicts.

Pushed as commit 63f8fc8.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants