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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 14 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
10a9b0e
8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar…
bradfordwetmore Mar 12, 2022
19b2d8a
Forgot copyright Info
bradfordwetmore Mar 12, 2022
151e2ee
Remove inadvertent whitespace
bradfordwetmore Mar 12, 2022
48f72ed
Minor test tweaks.
bradfordwetmore Mar 12, 2022
390f8e3
Merge
bradfordwetmore Mar 14, 2022
1c69b42
Merge branch 'master' into JDK-8273553
bradfordwetmore Mar 16, 2022
455f8e0
Merge branch 'master' into JDK-8273553
bradfordwetmore Mar 18, 2022
076a041
I/O Issues, rewrite the I/O section so that early Socket closes don't…
bradfordwetmore Mar 18, 2022
26de26a
Added SSLSocket bugid since we're actually checking both sides now.
bradfordwetmore Mar 18, 2022
4395301
Merge branch 'master' into JDK-8273553
bradfordwetmore Mar 18, 2022
9502279
Merge branch 'master' into JDK-8273553
bradfordwetmore Mar 23, 2022
b2f64d9
Code review comment: enclose conContext.closeInbound() in a try/final…
bradfordwetmore Mar 23, 2022
08d22ae
Merge branch 'master' into JDK-8273553
bradfordwetmore Mar 29, 2022
552637b
Merge branch 'master' into JDK-8273553
bradfordwetmore Apr 1, 2022
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
20 changes: 10 additions & 10 deletions src/java.base/share/classes/sun/security/ssl/SSLEngineImpl.java
View file
Open in desktop
Expand Up @@ -48,7 +48,7 @@
import javax.net.ssl.SSLSession;

/**
* Implementation of an non-blocking SSLEngine.
* Implementation of a non-blocking SSLEngine.
*
* @author Brad Wetmore
*/
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

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

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

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

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

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 •
edited

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

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 •
edited

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

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.

}
}

Expand Down
7 changes: 4 additions & 3 deletions src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java
View file
Open in desktop
Expand Up @@ -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();
Expand Down