RFR: 8259662: SocketException should be passed through

[Clive Verghese]

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

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

Commit messages:
 - 8259662: SocketException should be passed through

Changes: https://git.openjdk.java.net/jdk/pull/2057/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2057&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8259662
  Stats: 488 lines in 7 files changed: 479 ins; 0 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2057.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2057/head:pull/2057

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

[Daniel Fuchs-2]

On Wed, 13 Jan 2021 06:19:18 GMT, Clive Verghese <[hidden email]> wrote:

> 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

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

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

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

[Rajan Halade-2]

On Wed, 13 Jan 2021 06:19:18 GMT, Clive Verghese <[hidden email]> wrote:

> 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

Changes requested by rhalade (Reviewer).

test/jdk/sun/security/ssl/SSLContextImpl/ShouldThrowSSLExceptionWhenPeerClosesSocket.java line 32:

> 30:
> 31: /*
> 32:  * @test

Suggestion:

 * @test
 * @bug 8259662 8259516

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

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

[Clive Verghese]

> 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

Clive Verghese has updated the pull request incrementally with one additional commit since the last revision:

  Fix bugids and use server port passed as a parameter

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2057/files
  - new: https://git.openjdk.java.net/jdk/pull/2057/files/00667985..da98249d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2057&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2057&range=00-01

  Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2057.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2057/head:pull/2057

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

[Clive Verghese]

On Wed, 13 Jan 2021 18:41:26 GMT, Rajan Halade <[hidden email]> wrote:

>> Clive Verghese has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix bugids and use server port passed as a parameter
>
> test/jdk/sun/security/ssl/SSLContextImpl/ShouldThrowSSLExceptionWhenPeerClosesSocket.java line 32:
>
>> 30:
>> 31: /*
>> 32:  * @test
>
> Suggestion:
>
>  * @test
>  * @bug 8259662 8259516

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

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

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

[Clive Verghese]

> 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

Clive Verghese has updated the pull request incrementally with one additional commit since the last revision:

  Add error handling guidelines

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2057/files
  - new: https://git.openjdk.java.net/jdk/pull/2057/files/da98249d..4e715ba9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2057&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2057&range=01-02

  Stats: 10 lines in 1 file changed: 10 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2057.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2057/head:pull/2057

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

[Xue-Lei Andrew Fan]

On Wed, 13 Jan 2021 18:46:10 GMT, Rajan Halade <[hidden email]> wrote:

>> Clive Verghese has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
>>
>>  - Update copyright year
>>  - Add error handling guidelines
>>  - Fix bugids and use server port passed as a parameter
>>  - 8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl
>
> Changes requested by rhalade (Reviewer).

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.

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

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

[Clive Verghese]

On Wed, 27 Jan 2021 17:46:03 GMT, Xue-Lei Andrew Fan <[hidden email]> wrote:

>> Changes requested by rhalade (Reviewer).
>
> 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.

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

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

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

[Xue-Lei Andrew Fan]

On Tue, 26 Jan 2021 18:56:57 GMT, Clive Verghese <[hidden email]> wrote:

>> 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
>
> Clive Verghese has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
>
>  - Update copyright year
>  - Add error handling guidelines
>  - Fix bugids and use server port passed as a parameter
>  - 8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl

src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java line 1425:

> 1423:             } catch (SocketException se) {
> 1424:                 // don't change exception in case of SocketException
> 1425:                 throw se;

I may add the SocketException catch line together within line 1420.

src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java line 1491:

> 1489:             } catch (SocketException se) {
> 1490:                 // don't change exception in case of SocketException
> 1491:                 throw se;

I may add the SocketException catch line together within line 1489.

src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java line 1702:

> 1700:             if (!conContext.isInboundClosed()) {
> 1701:                 try {
> 1702:                     decode(null);

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?

src/java.base/share/classes/sun/security/ssl/SSLTransport.java line 146:

> 144:         } catch (SocketException se) {
> 145:             // don't change exception in case of SocketException
> 146:             throw se;

I may add the SocketException catch line together within line 141.

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

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

[Clive Verghese]

> 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

Clive Verghese has updated the pull request incrementally with one additional commit since the last revision:

  Address review comments

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2057/files
  - new: https://git.openjdk.java.net/jdk/pull/2057/files/72fef9b8..888162a4

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2057&range=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2057&range=05-06

  Stats: 15 lines in 2 files changed: 0 ins; 9 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2057.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2057/head:pull/2057

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

[Xue-Lei Andrew Fan]

On Thu, 28 Jan 2021 20:12:14 GMT, Clive Verghese <[hidden email]> wrote:

>> 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
>
> Clive Verghese has updated the pull request incrementally with one additional commit since the last revision:
>
>   Address review comments

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.

test/jdk/sun/security/ssl/SSLContextImpl/ShouldThrowSSLExceptionWhenPeerClosesSocket.java line 137:

> 135:             // Ignore exception as this is expected.
> 136:         }
> 137:     }

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

test/jdk/sun/security/ssl/SSLSocketImpl/SocketExceptionForSocketIssues.java line 84:

> 82:         } catch (SocketException se) {
> 83:             // the expected exception, ignore it
> 84:             System.err.println("server exception: " + se);

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

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

Changes requested by xuelei (Reviewer).

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

[Clive Verghese]

On Thu, 28 Jan 2021 19:04:29 GMT, Xue-Lei Andrew Fan <[hidden email]> wrote:

>> Clive Verghese has updated the pull request with a new target base due to a merge or a rebase.
>
> src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java line 1702:
>
>> 1700:             if (!conContext.isInboundClosed()) {
>> 1701:                 try {
>> 1702:                     decode(null);
>
> 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?

I have removed issues for JDK-8259516.

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

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

[Clive Verghese]

On Fri, 29 Jan 2021 00:50:04 GMT, Xue-Lei Andrew Fan <[hidden email]> wrote:

>> Clive Verghese has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Address review comments
>
> 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.

Hi,

Thank you for the feedback.

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

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

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

[Xue-Lei Andrew Fan]

On Tue, 2 Feb 2021 01:15:25 GMT, Clive Verghese <[hidden email]> wrote:

> 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.

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

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

[Clive Verghese]

> 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

Clive Verghese has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits:

 - Merge master
 - Fix test failures
 - Address review comments
 - Update copyright year
 - Add error handling guidelines
 - Fix bugids and use server port passed as a parameter
 - 8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl

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

Changes: https://git.openjdk.java.net/jdk/pull/2057/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2057&range=09
  Stats: 199 lines in 7 files changed: 177 ins; 0 del; 22 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2057.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2057/head:pull/2057

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

[Clive Verghese]

On Wed, 13 Jan 2021 06:19:18 GMT, Clive Verghese <[hidden email]> wrote:

> 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

This pull request has now been integrated.

Changeset: 63f8fc87
Author:    Clive Verghese <[hidden email]>
Committer: Xue-Lei Andrew Fan <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/63f8fc87
Stats:     199 lines in 7 files changed: 177 ins; 0 del; 22 mod

8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl

Reviewed-by: xuelei

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

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