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

8303809: Dispose context in SPNEGO NegotiatorImpl #12920

Closed
wants to merge 5 commits into from

Conversation

alexeybakhtin
Copy link
Contributor

@alexeybakhtin alexeybakhtin commented Mar 8, 2023

This patch fixes a possible native memory leak in case of a custom native GSS provider.
The actual leak was reported in production.

sun/security/jgss, sun/security/krb5, sun/net/www/protocol/http jtreg tests are passed


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

  • JDK-8303809: Dispose context in SPNEGO NegotiatorImpl

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12920

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 8, 2023

👋 Welcome back abakhtin! 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 Mar 8, 2023
@openjdk
Copy link

openjdk bot commented Mar 8, 2023

@alexeybakhtin The following labels will be automatically applied to this pull request:

  • net
  • security

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

@openjdk openjdk bot added security security-dev@openjdk.org net net-dev@openjdk.org labels Mar 8, 2023
@mlbridge
Copy link

mlbridge bot commented Mar 8, 2023

Webrevs

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.

@wangweij it would be good if you (or someone from security libs) could review this PR too

if (context != null) {
context.dispose();
}
}catch (GSSException e) {
Copy link
Member

Choose a reason for hiding this comment

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

Trivially: please add space after }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Sure, I will fix

Comment on lines 522 to 525

public void disposeContext() {
// do nothing
}
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to have some comment explaining the purpose of this method. In particular, it would be good to state when (at which point) it is supposed to be called.
Also hopefully the AuthenticationInfo object remain valid and can still be used after disposeContext has been called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, AuthenticationInfo is still valid after disposeContext.
The Negotiator with the new context will be recreated if required https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/net/www/protocol/http/NegotiateAuthentication.java#L224

@dfuch
Copy link
Member

dfuch commented Mar 8, 2023

/reviewers 2 reviewer

@openjdk
Copy link

openjdk bot commented Mar 8, 2023

@dfuch
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

@djelinski
Copy link
Member

I'm not familiar with this code and don't know how to execute it, but since you mentioned native memory leak...

I'm assuming NativeGSSConext is the class that holds a reference to the native memory. The class has a cleaner that is supposed to release the memory. It was recently refactored in JDK-8284490, and I think this refactoring introduced the leak. See:
https://github.com/openjdk/jdk/blob/master/src/java.security.jgss/share/classes/sun/security/jgss/wrapper/NativeGSSContext.java#L374-L377
cleanable.clean() calls stub.deleteContext(pContext), but pContext is already zero.
Before ffca23a deleteContext was called before setting pContext to zero.

In general, when fixing native memory leaks, please focus on fixing broken cleaners, rather than manually disposing the memory.

@wangweij
Copy link
Contributor

wangweij commented Mar 8, 2023

@djelinski Not a cleaner or lambda export, when cleanable.clean() is called, is it using its own copy of pContext and it's not reset to zero?

@djelinski
Copy link
Member

ah, you're right, cleanable has its own copy of pContext. You can disregard my previous comment.

But then, is there any other place where we could be leaking native memory? As far as I could tell, this patch only releases the memory earlier, it doesn't fix any leaks. Am I missing something?

@wangweij
Copy link
Contributor

wangweij commented Mar 8, 2023

Maybe it's because it can be added into a cache or stored in currentServerCredentials? But then Daniel F asks if it can remain valid.

@wangweij
Copy link
Contributor

wangweij commented Mar 8, 2023

My 2 cents: I think the change to HttpURLConnection and AuthenticationInfo is unnecessarily big. If you only care about the releasing of the GSSContext, how about add a check inside NegotiatorImpl after initSecContext is called? Once the isEstablished flag returns true the GSSContext is no more useful and we can dispose it.

This might not be perfect, for example, if the context is not established yet but for some reason the nextToken method is not called. In that case, I hope the cleaner can take care of the cleanup, sooner or later.

@alexeybakhtin
Copy link
Contributor Author

@dfunch, @djelinski, @wangweij Thank you a lot for review and suggestions.
The customer uses its own native Kerberos provider. This provider can allocate up to 2 MB of native memory per GSSContext until they are disposed. An application can generate many contexts quickly and they are not cleaned up until GC collects them. It causes high native memory consumption. So, the main idea of the fix is to dispose context as soon as possible.
Unfortunately, in most cases, context is not yet established in the NegotiatorImpl::init method after initSecContext(). So, we have no right place in the NegotiatorImpl to dispose the context after NegotiatorImpl::init. This is the reason I have to move the initiation of context dispose into the HttpURLConnection.
sun/security/krb5/auto/HttpNegotiateServer.java and sun/security/krb5/auto/HttpsCB.java tests can be used to verify that NegotiatorImpl::disposeContext() is called correctly.

@djelinski
Copy link
Member

@alexeybakhtin thanks for adding more context. I believe @wangweij was suggesting adding context.dispose() after initSecContext in NegotiatorImpl::nextToken, which indeed looks like the right thing to do.

@wangweij
Copy link
Contributor

wangweij commented Mar 9, 2023

If context is not established after first call to initSecContext, will nextToken always be called? In case it's not called, which code change guarantees the context is disposed?

@wangweij
Copy link
Contributor

wangweij commented Mar 9, 2023

@alexeybakhtin thanks for adding more context. I believe @wangweij was suggesting adding context.dispose() after initSecContext in NegotiatorImpl::nextToken, which indeed looks like the right thing to do.

Yes, after both initSecContext calls. In either case it's possible the context has already been established.

@alexeybakhtin
Copy link
Contributor Author

Unfortunately, there is no guarantee NegotiatorImpl::nextToken will be called.
At least in both sun/security/krb5/auto/HttpNegotiateServer.java and sun/security/krb5/auto/HttpsCB.java tests context is not established after the first initSecContext and NegotiatorImpl::nextToken is not called at all.

@wangweij
Copy link
Contributor

Yes, you are right. I believe the reason is that once an 200 OK is received, no one cares to look at the token anymore.

disposeContext();
} catch(Exception ex) {
//dispose context silently
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this cleanup necessary here but not in nextToken()? If we don't do any cleanup here, will disposeContext() be called inside HttpURLConnection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GSSContext could be allocated in init() line 97 but fails with Exception in context.initSecContext(). In this case null Negotiator is returned https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/net/www/protocol/http/Negotiator.java#L71 to NegotiatorAuthenticator: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/net/www/protocol/http/NegotiateAuthentication.java#L224. So nobody can clean context from HttpURLConnection

if (negotiator != null) {
try {
negotiator.disposeContext();
}catch(IOException ioEx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a space before catch.

@@ -251,7 +251,7 @@ public void disposeContext() {
if (negotiator != null) {
try {
negotiator.disposeContext();
}catch(IOException ioEx) {
} catch(IOException ioEx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another whitespace is required after catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Fixed

@dfuch
Copy link
Member

dfuch commented Mar 14, 2023

Hi @alexeybakhtin, I will do some tests and come back to you here.

@@ -82,5 +82,7 @@ private static void finest(Exception e) {
logger.finest("NegotiateAuthentication: " + e);
}
}

public abstract void disposeContext() throws IOException;
Copy link
Member

@dfuch dfuch Mar 14, 2023

Choose a reason for hiding this comment

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

We have some old internal tests that extend Negotiator and that will fail when this change is pushed. I'd suggest to change this method as follows:

Suggested change
public abstract void disposeContext() throws IOException;
public void disposeContext() throws IOException { }

This should avoid breaking existing subclasses that do not implement this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Changed from abstract to empty.
sun/security/jgss sun/security/krb5 sun/net/www/protocol/http tests passed

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.

Thanks Alexey. Tests returned green. Good to go!

@alexeybakhtin
Copy link
Contributor Author

Thanks Alexey. Tests returned green. Good to go!

Thank you a lot for review

@openjdk
Copy link

openjdk bot commented Mar 14, 2023

@alexeybakhtin 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:

8303809: Dispose context in SPNEGO NegotiatorImpl

Reviewed-by: dfuchs, weijun

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 92 new commits pushed to the master branch:

  • 9f9ab02: 8303895: Simplify and clean up LinkFactory code
  • a00f5d2: 8303861: Error handling step timeouts should never be blocked by OnError and others
  • da044dd: 8300939: sun/security/provider/certpath/OCSP/OCSPNoContentLength.java fails due to network errors
  • c466cdf: 8299546: C2: MulLNode::mul_ring() wrongly returns bottom type due to casting errors with large numbers
  • 55aa122: 8304059: Use InstanceKlass in dependencies
  • ec1eb00: 8303415: Add VM_Version::is_intrinsic_supported(id)
  • 31680b2: 8303410: Remove ContentSigner APIs and jarsigner -altsigner and -altsignerpath options
  • 0cc0f06: 8304015: G1: Metaspace-induced GCs should not trigger maximal compaction
  • 43eca1d: 8303910: jdk/classfile/CorpusTest.java failed 1 of 6754 tests
  • b6d70f2: 8303973: Library detection in runtime/ErrorHandling/TestDwarf.java fails on ppc64le RHEL8.5 for libpthread-2.28.so
  • ... and 82 more: https://git.openjdk.org/jdk/compare/afda8fbf0bcea18cbe741e9c693789ebe0c6c4c5...master

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 Mar 14, 2023
@alexeybakhtin
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Mar 14, 2023

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

  • 9f9ab02: 8303895: Simplify and clean up LinkFactory code
  • a00f5d2: 8303861: Error handling step timeouts should never be blocked by OnError and others
  • da044dd: 8300939: sun/security/provider/certpath/OCSP/OCSPNoContentLength.java fails due to network errors
  • c466cdf: 8299546: C2: MulLNode::mul_ring() wrongly returns bottom type due to casting errors with large numbers
  • 55aa122: 8304059: Use InstanceKlass in dependencies
  • ec1eb00: 8303415: Add VM_Version::is_intrinsic_supported(id)
  • 31680b2: 8303410: Remove ContentSigner APIs and jarsigner -altsigner and -altsignerpath options
  • 0cc0f06: 8304015: G1: Metaspace-induced GCs should not trigger maximal compaction
  • 43eca1d: 8303910: jdk/classfile/CorpusTest.java failed 1 of 6754 tests
  • b6d70f2: 8303973: Library detection in runtime/ErrorHandling/TestDwarf.java fails on ppc64le RHEL8.5 for libpthread-2.28.so
  • ... and 82 more: https://git.openjdk.org/jdk/compare/afda8fbf0bcea18cbe741e9c693789ebe0c6c4c5...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Mar 14, 2023

@alexeybakhtin Pushed as commit 10f1674.

💡 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 net net-dev@openjdk.org security security-dev@openjdk.org
4 participants