Skip to content

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

Closed
cliveverghese wants to merge 7 commits into
openjdk:masterfrom
cliveverghese:8259662
Closed

8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl#2057
cliveverghese wants to merge 7 commits into
openjdk:masterfrom
cliveverghese:8259662

Conversation

@cliveverghese

@cliveverghese cliveverghese commented Jan 13, 2021 •
edited by openjdk Bot
Loading

Copy link
Copy Markdown
Contributor

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

bridgekeeper Bot commented Jan 13, 2021

Copy link
Copy Markdown

👋 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 Pull request is ready for review label Jan 13, 2021
@openjdk

openjdk Bot commented Jan 13, 2021

Copy link
Copy Markdown

@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

mlbridge Bot commented Jan 13, 2021 •
edited
Loading

Copy link
Copy Markdown

Webrevs

dfuch
dfuch reviewed Jan 13, 2021
View reviewed changes

@dfuch dfuch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

cliveverghese commented Jan 13, 2021

Copy link
Copy Markdown
Contributor Author

/issue add JDK-8259516

@openjdk

openjdk Bot commented Jan 13, 2021

Copy link
Copy Markdown

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

rhalade
rhalade suggested changes Jan 13, 2021
View reviewed changes
Comment thread test/jdk/sun/security/ssl/SSLContextImpl/ShouldThrowSSLExceptionWhenPeerClosesSocket.java Outdated

@rhalade rhalade Jan 13, 2021

Copy link
Copy Markdown
Member

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

@cliveverghese cliveverghese Jan 13, 2021

Copy link
Copy Markdown
Contributor Author

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 Compare January 26, 2021 18:53
@XueleiFan

XueleiFan commented Jan 27, 2021

Copy link
Copy Markdown
Member

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

cliveverghese commented Jan 27, 2021

Copy link
Copy Markdown
Contributor Author

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

XueleiFan
XueleiFan reviewed Jan 28, 2021
View reviewed changes
Comment thread src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java Outdated
Comment thread src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java Outdated
Comment thread src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java Outdated

@XueleiFan XueleiFan Jan 28, 2021

Copy link
Copy Markdown
Member

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?

@cliveverghese cliveverghese Jan 29, 2021

Copy link
Copy Markdown
Contributor Author

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.

Comment thread src/java.base/share/classes/sun/security/ssl/SSLTransport.java Outdated
XueleiFan
XueleiFan suggested changes Jan 29, 2021
View reviewed changes

@XueleiFan XueleiFan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread test/jdk/sun/security/ssl/SSLContextImpl/ShouldThrowSSLExceptionWhenPeerClosesSocket.java Outdated
Comment on lines 130 to 137

@XueleiFan XueleiFan Jan 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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

Comment thread test/jdk/sun/security/ssl/SSLSocketImpl/SocketExceptionForSocketIssues.java Outdated
Comment on lines 79 to 84

@XueleiFan XueleiFan Jan 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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

cliveverghese commented Jan 29, 2021

Copy link
Copy Markdown
Contributor Author

/issue remove JDK-8259516

@openjdk

openjdk Bot commented Jan 29, 2021

Copy link
Copy Markdown

@cliveverghese
Removing additional issue from issue list: 8259516.

@cliveverghese

cliveverghese commented Jan 29, 2021

Copy link
Copy Markdown
Contributor Author

/test tier1

@openjdk

openjdk Bot commented Jan 29, 2021

Copy link
Copy Markdown

@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

cliveverghese commented Jan 29, 2021

Copy link
Copy Markdown
Contributor Author

/test tier2

@openjdk

openjdk Bot commented Jan 29, 2021

Copy link
Copy Markdown

@cliveverghese the test group tier2 does not exist

@cliveverghese

cliveverghese commented Feb 2, 2021

Copy link
Copy Markdown
Contributor Author

Hi,

Thank you for the feedback.

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

@XueleiFan

XueleiFan commented Feb 2, 2021

Copy link
Copy Markdown
Member

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

XueleiFan commented Feb 3, 2021

Copy link
Copy Markdown
Member

/csr needed

@openjdk openjdk Bot added the csr Pull request needs approved CSR before integration label Feb 3, 2021
@openjdk

openjdk Bot commented Feb 3, 2021

Copy link
Copy Markdown

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

XueleiFan
XueleiFan approved these changes Feb 3, 2021
View reviewed changes

@XueleiFan XueleiFan left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 Pull request needs approved CSR before integration label Feb 19, 2021
@openjdk

openjdk Bot commented Feb 19, 2021

Copy link
Copy Markdown

@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 openjdk Bot added the merge-conflict Pull request has merge conflict with target branch label Feb 19, 2021
@openjdk

openjdk Bot commented Feb 19, 2021 •
edited
Loading

Copy link
Copy Markdown

@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 Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Feb 19, 2021
@cliveverghese

cliveverghese commented Feb 20, 2021

Copy link
Copy Markdown
Contributor Author

/integrate

@openjdk openjdk Bot added the sponsor Pull request is ready to be sponsored label Feb 20, 2021
@openjdk

openjdk Bot commented Feb 20, 2021

Copy link
Copy Markdown

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

@XueleiFan

XueleiFan commented Feb 22, 2021

Copy link
Copy Markdown
Member

/sponsor

@openjdk openjdk Bot closed this Feb 22, 2021
@openjdk openjdk Bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 22, 2021
@openjdk

openjdk Bot commented Feb 22, 2021

Copy link
Copy Markdown

@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

Reviewers

3 more reviewers
@dfuch dfuch dfuch left review comments
@rhalade rhalade rhalade requested changes
@XueleiFan XueleiFan XueleiFan approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

integrated Pull request has been integrated net [email protected] security [email protected] test-request

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cliveverghese @XueleiFan @rhalade @dfuch