On Thu, 22 Apr 2021 04:13:54 GMT, Xue-Lei Andrew Fan <[email protected]> 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
