Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368 #7796

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -48,7 +48,7 @@
import javax.net.ssl.SSLSession;

/**
* Implementation of an non-blocking SSLEngine.
* Implementation of a non-blocking SSLEngine.
*
* @author Brad Wetmore
*/
@@ -270,7 +270,7 @@ private SSLEngineResult writeRecord(
if (ciphertext == null && !conContext.isNegotiated &&
conContext.isInboundClosed() &&
hsStatus == HandshakeStatus.NEED_WRAP) {
// Even the outboud is open, no futher data could be wrapped as:
// Even the outbound is open, no further data could be wrapped as:
// 1. the outbound is empty
// 2. no negotiated connection
// 3. the inbound has closed, cannot complete the handshake
@@ -789,17 +789,17 @@ public void closeInbound() throws SSLException {
// Is it ready to close inbound?
//
// No exception if the initial handshake is not started.
if (!conContext.isInputCloseNotified &&
(conContext.isNegotiated ||
conContext.handshakeContext != null)) {

throw conContext.fatal(Alert.INTERNAL_ERROR,
if (!conContext.isInputCloseNotified && (conContext.isNegotiated
|| conContext.handshakeContext != null)) {
throw new SSLException(
"closing inbound before receiving peer's close_notify");
}

conContext.closeInbound();
} finally {
engineLock.unlock();
try {
conContext.closeInbound();
} finally {
engineLock.unlock();
Copy link
Member

@dfuch dfuch Mar 23, 2022

Choose a reason for hiding this comment

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

I see that onContext.closeInbound() might throw, which would leave the engineLock locked and could cause deadlocks down the road. So maybe you should have a nested try { } finally { } here to make sure the lock is properly unlocked.

Copy link
Member

@XueleiFan XueleiFan Mar 23, 2022

Choose a reason for hiding this comment

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

+1.

Copy link
Contributor Author

@bradfordwetmore bradfordwetmore Mar 23, 2022

Choose a reason for hiding this comment

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

Good catch. Thanks.

}
Copy link
Member

@XueleiFan XueleiFan Mar 23, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@bradfordwetmore bradfordwetmore Mar 24, 2022

Choose a reason for hiding this comment

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

@XueleiFan, 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.

Copy link
Member

@XueleiFan XueleiFan Mar 24, 2022

Choose a reason for hiding this comment

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

@XueleiFan, I'm not following your first comment.

Sorry for that. I was wondering something new in the future, which is not doable in the current Java language. Please just leave it alone.

Copy link
Contributor Author

@bradfordwetmore bradfordwetmore Mar 24, 2022

Choose a reason for hiding this comment

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

Ah! Thanks.

Copy link
Member

@XueleiFan XueleiFan Mar 24, 2022

Choose a reason for hiding this comment

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

IIUC, you are suggesting:
... snipped ...

Yes.

What is the advantage to changing to this style, other than a very small performance win by not locking in the already closed case?

The advantage is mainly about the performance, as I/O of SSLLogger could be slow. The SSLSocketImpl code does not put the similar block locked. But it may be not the purpose of this update. I'm fine if you want to leave it alone.

Copy link
Contributor Author

@bradfordwetmore bradfordwetmore Apr 2, 2022

Choose a reason for hiding this comment

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

I can see your point for these two methods (close*bound()), but we are using SSLLogger behind the engineLock throughout the implementation. I think I will leave as is, rather than try to squeak this little bit out when logging is on and active.

}
}

@@ -838,9 +838,10 @@ private void shutdownInput(
// No need to throw exception if the initial handshake is not started.
try {
if (checkCloseNotify && !conContext.isInputCloseNotified &&
(conContext.isNegotiated || conContext.handshakeContext != null)) {
throw new SSLException(
"closing inbound before receiving peer's close_notify");
(conContext.isNegotiated ||
conContext.handshakeContext != null)) {
throw new SSLException(
"closing inbound before receiving peer's close_notify");
}
} finally {
conContext.closeInbound();