RFR: 8168469: Memory leak in JceSecurity

[Valerie Peng]

On Tue, 25 Apr 2023 21:16:41 GMT, Sergey Chernyshev <duke at openjdk.org> wrote:

> 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] https://github.com/openjdk/jdk/commit/74d45e481d1ad6aa5d7c6315ea86681e1a978ce0

Rest looks good to me. Thanks!

src/java.base/share/classes/javax/crypto/JceSecurity.java.template line 217:

> 215:                         // recursion; return failure now
> 216:                         throw new IllegalStateException
> 217:                                 ("Recursion during verification");

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

-------------

Marked as reviewed by valeriep (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13658#pullrequestreview-1419581596
PR Review Comment: https://git.openjdk.org/jdk/pull/13658#discussion_r1189220726