RFR: 8263779: SSLEngine reports NEED_WRAP continuously without producing any further output [v2]

[Bradford Wetmore]

On Thu, 22 Apr 2021 04:13:54 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> As described in the bug, by connecting the SSLEngine with a misbehaving peer SSL implementation, it can get into a state where it calling `wrap` reports getStatus == OK, getHandshakeStatus === NEED_WRAP but still doesn't produce any further output.   It happens when the output bound is not empty.
>> 
>> It is caused by a mismatching condition in the SSLEngineOutputRecord.  The use hasAlert() method should be replaced with isEmpty().  Otherwise, there is conflicts of the closing status while checking with OutputRecord.isEmpty() in TransportContext.getHandshakeStatus() implementation.  It is safe to remove hasAlert() method, as we don't allow creation of new output record if the closure is in progress, thus isEmpty() could be used instead.
>> 
>> The patch passed the test provided by the bug submitter.
>
> Xue-Lei Andrew Fan has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge
>  - improved fix
>  - 8263779: SSLEngine reports NEED_WRAP continuously without producing any further output

SSLEngineOutputRecord.java:
Missing copyright date update.

SSLEngineOutputRecord:77
Unnecessary extra line added.  Please remove.

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

> 1692:         if (cause instanceof SocketException) {
> 1693:             try {
> 1694:                 throw conContext.fatal(alert, cause);

Why did you change to a throw here?  Isn't everything from fatal() going to be thrown anyway?

src/java.base/share/classes/sun/security/ssl/TransportContext.java line 587:

> 585:     }
> 586: 
> 587:     // Note: HandshakeStatus.FINISHED status is retrieved in other places.

I now see that v1 of this change had quite a few changes here, and what's left is now just compacting a line and making some old comment corrections.

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

Marked as reviewed by wetmore (Reviewer).

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