RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368 [v2]

[Bradford Wetmore]

On Wed, 23 Mar 2022 18:09:46 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> Bradford Wetmore 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 12 additional commits since the last revision:
>> 
>>  - Code review comment: enclose conContext.closeInbound() in a try/finally block.
>>  - Merge branch 'master' into JDK-8273553
>>  - Merge branch 'master' into JDK-8273553
>>  - Added SSLSocket bugid since we're actually checking both sides now.
>>  - I/O Issues, rewrite the I/O section so that early Socket closes don't kill our server-side reads.
>>  - Merge branch 'master' into JDK-8273553
>>  - Merge branch 'master' into JDK-8273553
>>  - Merge
>>  - Minor test tweaks.
>>  - Remove inadvertent whitespace
>>  - ... and 2 more: https://git.openjdk.java.net/jdk/compare/fdf65be0...b2f64d92
>
> src/java.base/share/classes/sun/security/ssl/SSLEngineImpl.java line 802:
> 
>> 800:             } finally {
>> 801:                 engineLock.unlock();
>> 802:             }
> 
> I looked the update further.  It would be nice if the try-statement could support more than one finally blocks in the future so that the code could be more clear.
> 
> try {
>     ...
> } finally {
>    // the 1st final block
> } finally {
>    // the 2nd final block
> }
> 
> 
> For the block from 781 to 787, it may be more effective if moving the block out of the synchronization lock (that's, moving 781-787 to line 779.)

I'm not following your first comment.  AFAIK, double finally blocks aren't currently an option, unless I'm missing a new language feature.  Or is this just a general "it would be nice if the Java language was able to do..." comment?

        try {
            ...
            throw new SSLException(...);
        } finally {
            conContext.closeInbound();
        } finally {
            engineLock.unlock();
        }

As to your second question, the model of:

method() {
    engineLock.lock();
    try {
        action();
    } finally {
        engineUnlock.unlock();
    }
}

is very simple to understand and thus maintain, and is used quite consistently throughout SSLEngineImpl and catches any problems.  IIUC, you are suggesting:

    public void closeInbound() throws SSLException {
        if (isInboundDone()) {
            return;
        }

        if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
            SSLLogger.finest("Closing inbound of SSLEngine");
        }

        engineLock.lock();
        try {
            if (!conContext.isInputCloseNotified && (conContext.isNegotiated
                    || conContext.handshakeContext != null)) {
                throw new SSLException(
                        "closing inbound before receiving peer's close_notify");
            }
        } finally {
            try {
                conContext.closeInbound();
            } finally {
                engineLock.unlock();
            }
        }
    }

What is the advantage to changing to this style, other than a very small performance win by not locking in the already closed case?  Isn't the locking code pretty optimized?  SSLLogger might throw a NPE (unlikely), and isInboundDone() just checks a variable, but the code could change down the road.  I'm just not seeing a reason to change the maintainability of this code.

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

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