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

8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException #13742

Closed
wants to merge 10 commits into from

Conversation

mpdonova
Copy link
Contributor

@mpdonova mpdonova commented May 1, 2023

In this PR, I added methods to the TransportContext class to synchronize access to the handshakeContext field. I also updated locations in the code that rely on the handshakeContext field to not be null to use the synchronized methods.

Thanks


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13742/head:pull/13742
$ git checkout pull/13742

Update a local copy of the PR:
$ git checkout pull/13742
$ git pull https://git.openjdk.org/jdk.git pull/13742/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13742

View PR using the GUI difftool:
$ git pr show -t 13742

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13742.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented May 1, 2023

👋 Welcome back mpdonova! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label May 1, 2023
@openjdk
Copy link

openjdk bot commented May 1, 2023

@mpdonova The following label will be automatically applied to this pull request:

  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the security security-dev@openjdk.org label May 1, 2023
@mlbridge
Copy link

mlbridge bot commented May 1, 2023

Webrevs

Comment on lines 766 to 767
!conContext.handshakeContext.taskDelegated &&
!conContext.handshakeContext.delegatedActions.isEmpty()) {
Copy link
Member

@dfuch dfuch May 2, 2023

Choose a reason for hiding this comment

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

Shouldn't you replace conContext.handshakeContext with context in those two lines as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I definitely should have done that. Thanks.

Comment on lines 459 to 461
handshakeCtxLock.lock();
handshakeContext = null;
handshakeCtxLock.unlock();
Copy link
Member

Choose a reason for hiding this comment

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

The usual pattern is to use try { } finally { } with locks (even though in this particular case I don't see how the assignment would throw) - but if more things need to be done in the future here then at least the try { } finally { } will be in place.

Suggested change
handshakeCtxLock.lock();
handshakeContext = null;
handshakeCtxLock.unlock();
handshakeCtxLock.lock();
try {
handshakeContext = null;
} finally {
handshakeCtxLock.unlock();
}

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 added the try/finally.

@@ -89,6 +88,7 @@ final class TransportContext implements ConnectionContext {

// handshake context
HandshakeContext handshakeContext = null;
Lock handshakeCtxLock = new ReentrantLock();
Copy link
Member

Choose a reason for hiding this comment

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

handshakeCtxLock should be declared final.

Copy link
Member

@dfuch dfuch left a comment

Choose a reason for hiding this comment

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

LGTM. You will need approval from a security-dev Reviewer before pushing though.
In particular I am not sure about the implication of removing the lock around SSLEngineIml::getHandshakeSession() and SSLSocketImpl::getHandshakeApplicationProtocols()

@mpdonova
Copy link
Contributor Author

mpdonova commented May 8, 2023

Can a security-dev Reviewer take a look at this? Thanks!

@mpdonova
Copy link
Contributor Author

Hi, I'm still looking for a Reviewer for this PR. Thanks!

Copy link
Member

@jnimeh jnimeh left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@openjdk
Copy link

openjdk bot commented Jun 16, 2023

@mpdonova This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException

Reviewed-by: jnimeh, xuelei

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the master branch:

  • 19691fa: 6361826: (reflect) provide method for mapping strings to class object for primitive types
  • c84866a: 8310551: vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java timed out due to missing prompt
  • 0d2196f: 8311992: Test java/lang/Thread/virtual/JfrEvents::testVirtualThreadPinned failed

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 16, 2023
@@ -911,13 +912,7 @@ public SSLSession getSession() {

@Override
public SSLSession getHandshakeSession() {
engineLock.lock();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not very sure of this update. By removing the enginLock on socket/engine level here, is it possible there are two threads that one read the handshake session and another on write?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

engineLock was used to here to make sure that conContext.handshakeContext is not null before accessing it. The problem is that the handshakeContext is modified (set to null) by the TransportContext class. So using a lock in SSLEngineImpl or SSLSocketImpl doesn't protect handshakeContext from becoming null between the check and use.

The actual SSLSession object was never protected from concurrent modification (unless it has its own lock.)

When reviewing the code to answer this, I noticed that I needed to update SSLEngineImpl.getHandshakeApplicationProtocol() so there is a new commit.

Copy link
Member

Choose a reason for hiding this comment

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

I had a search of the value assignment of handshakeSession. Here is one example of the calling stack:
SSLEngineImpl.DelegatedTask.run()->TransportContext.dispatch()->SSLHandshake().consume()->HandshakeConsumer.consume()->SSLExtensions.product()->ServerHello.produce()->shc.handshakeSession = session

The engineLock was placed on SSLEngineImpl.DelegatedTask.run() and released after complete the job, which means the value assignment of handshakeSession is synchronized.

The locks in SSLEngineImpl and SSLSocketImpl are used for operations synchronization so that other classes may not need additional locks in most cases.

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 see what you're saying with respect to SSLEngineImpl. It looks like all of the public methods are synchronized with engineLock so I shouldn't have made any changes there. I will revert them.

Are you ok with the changes in SSLSocketImpl?

Copy link
Member

Choose a reason for hiding this comment

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

I may update both SSLSocketImpl and SSLEngineImpl, as SSLSocketImpl uses the locks similar to SSLEngineImpl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay on any updates here.

I updated this branch and verified the tests still pass. I ran jdk_security3 tests from test/jdk/TEST.groups. Is there anything else I should do to test this change?

Copy link
Member

Choose a reason for hiding this comment

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

SSLSocketImpl uses the locks similar to SSLEngineImpl. It may get it complicated and hard to maintain if using different locks in SSLSocketImpl and SSLEngineImpl. You may be able to find similar locking scenarios in SSLSocket implementation like: SSLEngineImpl.DelegatedTask.run()->TransportContext.dispatch()->SSLHandshake().consume()->HandshakeConsumer.consume()->SSLExtensions.product()->ServerHello.produce()->shc.handshakeSession = session.

I understand where you came from for the NullPointerException in the bug. But it is hardly the direction to change the overall locking scenarios. Maybe, the close() function implementation could be focused on instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only lock I added was in TransportContext to synchronize access to the handshakeContext field, but I understand your reluctance to make any changes with locks. The problem is that SSLSocketImpl tries to access conContext.handshakeContext.negotiatedProtocol after TransportContext has set handshakeContext to null.

TransportContext also has a protocolVersion field. Is it possible to just use that instead?

Copy link
Member

Choose a reason for hiding this comment

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

TransportContext also has a protocolVersion field. Is it possible to just use that instead?

Did you mean a change in duplexCloseOutput() like the following?

-           // The protocol version may have been negotiated.
-           ProtocolVersion pv = conContext.handshakeContext.negotiatedProtocol;
+           // The protocol version may have been negotiated.  The 
+           // conContext.handshakeContext.negotiatedProtocol is not used as there
+           // may be a race to set it to null.
+          ProtocolVersion pv = conContext.protocolVersion;
            if (pv == null || (!pv.useTLS13PlusSpec())) {
                hasCloseReceipt = true;
            }       

I like the idea as it looks like a safe update in the current implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is what I was thinking.I will make the change and test it out. Thanks.

Copy link
Member

@XueleiFan XueleiFan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

@mpdonova
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Jul 17, 2023

Going to push as commit afcf8e4.
Since your change was applied there have been 10 commits pushed to the master branch:

  • acf591e: 8140527: JInternalFrame has incorrect title button width
  • f6e23ae: 8310201: Reduce verbose locale output in -XshowSettings launcher option
  • a441216: 8311815: Incorrect exhaustivity computation
  • 1c9691b: 8312093: Incorrect javadoc comment text
  • 37ca902: 8310233: Fix THP detection on Linux
  • 81c4e8f: 8304006: jlink should create the jimage file in the native endian for the target platform
  • e8f66bf: 8310949: RISC-V: Initialize UseUnalignedAccesses
  • 19691fa: 6361826: (reflect) provide method for mapping strings to class object for primitive types
  • c84866a: 8310551: vmTestbase/nsk/jdb/interrupt/interrupt001/interrupt001.java timed out due to missing prompt
  • 0d2196f: 8311992: Test java/lang/Thread/virtual/JfrEvents::testVirtualThreadPinned failed

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jul 17, 2023
@openjdk openjdk bot closed this Jul 17, 2023
@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Jul 17, 2023
@openjdk openjdk bot removed the rfr Pull request is ready for review label Jul 17, 2023
@openjdk
Copy link

openjdk bot commented Jul 17, 2023

@mpdonova Pushed as commit afcf8e4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated security security-dev@openjdk.org
4 participants