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

8168469: Memory leak in JceSecurity #13658

Closed
wants to merge 2 commits into from
Closed

Conversation

sercher
Copy link
Contributor

@sercher sercher commented Apr 25, 2023

Hi all,

I would like to propose a patch for an issue discussed in [1][2] that fixes an OOME in the current code base. The issue appears when a SunJCE provider object reference is stored in a non-static variable, which eventually leads to a Java heap OOME.

The solution proposed earlier [1] raised a concern that the identity of provider objects may be somehow broken when using WeakHashMap<Class<? extends Provider>, Object> instead of IdentityHashMap<Provider, Object>. The solution hasn't been integrated that time. Later in 2019 a performance improvement was introduced with [3][4] that changed IdentityHashMap to ConcurrentHashMap of IdentityWrapper<Provider> objects that maintained the object identity while improved performance.

The solution being proposed keeps up with performance improvement in [3], further narrowing the synchronization down to the hash table node, avoids lambdas that may cause startup time regressions and maintains providers identity using WeakIdentityWrapper that extends WeakReference<Object> while avoiding depletion of Java heap by using weak pointers as the mapping key. Once a provider object becomes weakly reachable it is queued along with its reference object to the ReferenceQueue (a new static member in JceSecurity). The equals() method of the WeakIdentityWrapper will never match a new wrapper object to anything inside the hash table after the corresponding element's WeakReference.get() returned null. This leads to accumulating "empty" elements in the hash table. The new static function expungeStaleWrappers() drops the "empty" elements queued by GC each time the getVerificationResult() method is called.

ConcurrentHashMap's computeIfAbsent() does (partially) detect recursive updates for keys that are being added to empty bins. For such keys an IllegalStateException is thrown prior to recursive execution of the mappingFunction. For nodes that are added to existing TreeBins or linked lists, in which case no recursion detection is done prior to calling mappingFunction, the recursion detection relies on old method with IdentityMap of Providers.

I include a test that runs under memory constrained conditions (128M) that hits the heap limit in current VMs where it is impossible to reclaim providers' memory (unless they've been cached under weak references). The updated jdk versions pass the test.

Testing: jtreg + JCK on a downport of the patch to JDK 17 with no regressions

[1] https://mail.openjdk.org/pipermail/security-dev/2016-October/015024.html
[2] https://bugs.openjdk.org/browse/JDK-8168469
[3] https://bugs.openjdk.org/browse/JDK-7107615
[4] 74d45e4


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

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13658

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 25, 2023

👋 Welcome back sercher! 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 Apr 25, 2023
@openjdk
Copy link

openjdk bot commented Apr 25, 2023

@sercher 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 Apr 25, 2023
@mlbridge
Copy link

mlbridge bot commented Apr 25, 2023

Webrevs

@sercher
Copy link
Contributor Author

sercher commented May 4, 2023

@valeriepeng Could you please take a look at this change?

@valeriepeng
Copy link

Ok, I will take a look.

if (verifyingProviders.get(p) != null) {
// recursion; return failure now
return new NoSuchProviderException
throw new IllegalStateException
("Recursion during verification");

Choose a reason for hiding this comment

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

nit: no need for repeating error message here since it's not really used in line 242 anyway.

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! I updated the PR.

Copy link

@valeriepeng valeriepeng left a comment

Choose a reason for hiding this comment

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

Rest looks good to me. Thanks!

@openjdk
Copy link

openjdk bot commented May 9, 2023

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

8168469: Memory leak in JceSecurity

Reviewed-by: valeriep

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

  • 7455bb2: 8308015: Syntax of "import static" is incorrect in com.sun.source.tree.ImportTree.java
  • 6ebea89: 8307911: javadoc for MemorySegment::reinterpret has duplicate restricted method paragraph
  • 3c68c35: 8307535: java.util.logging.Handlers should be more VirtualThread friendly
  • 9fa8b9a: 8307409: Refactor usage examples to use @snippet in the java.nio packages
  • e512a20: 8306541: Refactor collection set candidate handling to prepare for JDK-8140326
  • 4b0f421: 8307855: update for deprecated sprintf for src/utils
  • 39dc40f: 8305081: Remove finalize() from test/hotspot/jtreg/compiler/runtime/Test8168712
  • f7bbbc6: 8307808: G1: Remove partial object-count report after gc
  • 13a3fce: 8307961: java/foreign/enablenativeaccess/TestEnableNativeAccess.java fails with ShouldNotReachHere
  • f09a0f5: 8307806: Rename Atomic::fetch_and_add and friends
  • ... and 275 more: https://git.openjdk.org/jdk/compare/d819debaa5f0155e5e3990fa4f919ab420610c97...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@valeriepeng) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 9, 2023
@sercher
Copy link
Contributor Author

sercher commented May 11, 2023

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label May 11, 2023
@openjdk
Copy link

openjdk bot commented May 11, 2023

@sercher
Your change (at version ffea537) is now ready to be sponsored by a Committer.

@valeriepeng
Copy link

/sponsor

@openjdk
Copy link

openjdk bot commented May 12, 2023

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

  • 7455bb2: 8308015: Syntax of "import static" is incorrect in com.sun.source.tree.ImportTree.java
  • 6ebea89: 8307911: javadoc for MemorySegment::reinterpret has duplicate restricted method paragraph
  • 3c68c35: 8307535: java.util.logging.Handlers should be more VirtualThread friendly
  • 9fa8b9a: 8307409: Refactor usage examples to use @snippet in the java.nio packages
  • e512a20: 8306541: Refactor collection set candidate handling to prepare for JDK-8140326
  • 4b0f421: 8307855: update for deprecated sprintf for src/utils
  • 39dc40f: 8305081: Remove finalize() from test/hotspot/jtreg/compiler/runtime/Test8168712
  • f7bbbc6: 8307808: G1: Remove partial object-count report after gc
  • 13a3fce: 8307961: java/foreign/enablenativeaccess/TestEnableNativeAccess.java fails with ShouldNotReachHere
  • f09a0f5: 8307806: Rename Atomic::fetch_and_add and friends
  • ... and 275 more: https://git.openjdk.org/jdk/compare/d819debaa5f0155e5e3990fa4f919ab420610c97...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented May 12, 2023

@valeriepeng @sercher Pushed as commit a284920.

💡 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
2 participants