Ensure BoringSSLAsymmetricCipherKeyPair will eventually release nativ…

…e memory Motivation: How we tried to release native memory did not work for various reasons. Modifications: Ensure native memory is released Result: No more native memory leak causes by keypairs
netty · Jul 16, 2024 · b6f8d8d · b6f8d8d
1 parent 549dc19
commit b6f8d8d
Show file tree

Hide file tree

Showing 7 changed files with 23 additions and 43 deletions.
diff --git a/...boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLAEADContext.java b/...boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLAEADContext.java
@@ -63,8 +63,8 @@ int execute(long ctx, ByteBufAllocator alloc, long ad, int adLen, long in, int i
         }
     };
 
-    BoringSSLAEADContext(BoringSSLOHttpCryptoProvider cryptoProvider, long ctx, int aeadMaxOverhead, byte[] baseNonce) {
-        super(cryptoProvider, ctx);
+    BoringSSLAEADContext(long ctx, int aeadMaxOverhead, byte[] baseNonce) {
+        super(ctx);
         this.nonce = new Nonce(baseNonce);
         this.aeadMaxOverhead = aeadMaxOverhead;
     }

diff --git a/...c/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLAsymmetricCipherKeyPair.java b/...c/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLAsymmetricCipherKeyPair.java
@@ -16,7 +16,6 @@
 package io.netty.incubator.codec.hpke.boringssl;
 
 import io.netty.incubator.codec.hpke.AsymmetricCipherKeyPair;
-import io.netty.incubator.codec.hpke.AsymmetricKeyParameter;
 
 // TODO: Maybe expose sub-type which is ReferenceCounted and so allows to take ownership of EVP_HPKE_KEY.
 final class BoringSSLAsymmetricCipherKeyPair implements AsymmetricCipherKeyPair {
@@ -77,5 +76,16 @@ public String toString() {
                 ", publicKey=" + publicKey +
                 '}';
     }
+
+    @Override
+    protected void finalize() throws Throwable {
+        try {
+            if (key != -1) {
+                BoringSSL.EVP_HPKE_KEY_cleanup_and_free(key);
+            }
+        } finally {
+            super.finalize();
+        }
+    }
 }
 
diff --git a/...ringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLCryptoContext.java b/...ringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLCryptoContext.java
@@ -24,13 +24,10 @@
  */
 abstract class BoringSSLCryptoContext implements CryptoContext {
 
-    private final BoringSSLOHttpCryptoProvider cryptoProvider;
-
     // We use an AtomicLong to reduce the possibility of crashing after the user called close().
     private final AtomicLong ctxRef;
 
-    BoringSSLCryptoContext(BoringSSLOHttpCryptoProvider cryptoProvider, long ctx) {
-        this.cryptoProvider = cryptoProvider;
+    BoringSSLCryptoContext(long ctx) {
         assert ctx != -1;
         this.ctxRef = new AtomicLong(ctx);
     }
@@ -54,7 +51,6 @@ public final void close()  {
        long ctx = ctxRef.getAndSet(-1);
        if (ctx != -1) {
            destroyCtx(ctx);
-           cryptoProvider.free_EVP_HPKE_KEYS();
        }
     }
 

diff --git a/...boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKEContext.java b/...boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKEContext.java
@@ -24,8 +24,8 @@ class BoringSSLHPKEContext extends BoringSSLCryptoContext implements HPKEContext
 
     private final long digest;
 
-    BoringSSLHPKEContext(BoringSSLOHttpCryptoProvider cryptoProvider, long hpkeCtx) {
-        super(cryptoProvider, hpkeCtx);
+    BoringSSLHPKEContext(long hpkeCtx) {
+        super(hpkeCtx);
         digest = BoringSSL.EVP_HPKE_KDF_hkdf_md(BoringSSL.EVP_HPKE_CTX_kdf(hpkeCtx));
     }
 

diff --git a/.../src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKERecipientContext.java b/.../src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKERecipientContext.java
@@ -41,9 +41,9 @@ int execute(long ctx, ByteBufAllocator alloc,  long ad, int adLen, long in, int
     // Store a reference to the keyPair here so we are sure it will only be GCed once the context is collected as well.
     final AsymmetricCipherKeyPair keyPair;
 
-    BoringSSLHPKERecipientContext(BoringSSLOHttpCryptoProvider cryptoProvider, long hpkeCtx,
+    BoringSSLHPKERecipientContext(long hpkeCtx,
                                   AsymmetricCipherKeyPair keyPair) {
-        super(cryptoProvider, hpkeCtx);
+        super(hpkeCtx);
         this.keyPair = keyPair;
     }
 

diff --git a/...ssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKESenderContext.java b/...ssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKESenderContext.java
@@ -41,8 +41,8 @@ int execute(long ctx, ByteBufAllocator alloc, long ad, int adLen, long in, int i
 
     private final byte[] encapsulation;
 
-    BoringSSLHPKESenderContext(BoringSSLOHttpCryptoProvider cryptoProvider, long hpkeCtx, byte[] encapsulation) {
-        super(cryptoProvider, hpkeCtx);
+    BoringSSLHPKESenderContext(long hpkeCtx, byte[] encapsulation) {
+        super(hpkeCtx);
         this.encapsulation = encapsulation;
     }
 

diff --git a/...l/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLOHttpCryptoProvider.java b/...l/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLOHttpCryptoProvider.java
@@ -26,8 +26,6 @@
 import io.netty.incubator.codec.hpke.KEM;
 import io.netty.incubator.codec.hpke.OHttpCryptoProvider;
 
-import java.lang.ref.PhantomReference;
-import java.lang.ref.ReferenceQueue;
 import java.util.Arrays;
 
 /**
@@ -37,16 +35,6 @@
  */
 public final class BoringSSLOHttpCryptoProvider implements OHttpCryptoProvider {
 
-    private final ReferenceQueue<BoringSSLAsymmetricCipherKeyPair> keyPairRefQueue = new ReferenceQueue<>();
-    private static final class EVP_HPKE_KEY_PhantomRef extends PhantomReference<BoringSSLAsymmetricCipherKeyPair> {
-        private final long key;
-        EVP_HPKE_KEY_PhantomRef(BoringSSLAsymmetricCipherKeyPair referent,
-                                       ReferenceQueue<BoringSSLAsymmetricCipherKeyPair> q) {
-            super(referent, q);
-            this.key = referent.key;
-        }
-    }
-
     /**
      * {@link BoringSSLOHttpCryptoProvider} instance.
      */
@@ -70,7 +58,7 @@ public AEADContext setupAEAD(AEAD aead, byte[] key, byte[] baseNonce) {
         int maxOverhead = BoringSSL.EVP_AEAD_max_overhead(boringSSLAead);
         long ctx = BoringSSL.EVP_AEAD_CTX_new_or_throw(boringSSLAead, key, BoringSSL.EVP_AEAD_DEFAULT_TAG_LENGTH);
         try {
-            BoringSSLAEADContext aeadCtx = new BoringSSLAEADContext(this, ctx, maxOverhead, baseNonce);
+            BoringSSLAEADContext aeadCtx = new BoringSSLAEADContext(ctx, maxOverhead, baseNonce);
             ctx = -1;
             return aeadCtx;
         } finally {
@@ -144,7 +132,7 @@ public HPKESenderContext setupHPKEBaseS(KEM kem, KDF kdf, AEAD aead, AsymmetricK
                 throw new IllegalStateException("Unable to setup EVP_HPKE_CTX");
             }
             BoringSSLHPKESenderContext hpkeCtx =
-                    new BoringSSLHPKESenderContext(this, ctx, encapsulation);
+                    new BoringSSLHPKESenderContext(ctx, encapsulation);
             ctx = -1;
             return hpkeCtx;
         } finally {
@@ -191,7 +179,7 @@ public HPKERecipientContext setupHPKEBaseR(KEM kem, KDF kdf, AEAD aead, byte[] e
 
             // Store a reference to the keyPair so we are sure it will only be GCed once the context is collected as
             // well. This ensures the key is not added to the reference queue before the context is destroyed as well.
-            BoringSSLHPKERecipientContext hpkeCtx = new BoringSSLHPKERecipientContext(this, ctx, skR);
+            BoringSSLHPKERecipientContext hpkeCtx = new BoringSSLHPKERecipientContext(ctx, skR);
             ctx = -1;
             return hpkeCtx;
         } finally {
@@ -220,7 +208,6 @@ public AsymmetricCipherKeyPair deserializePrivateKey(KEM kem, byte[] privateKeyB
             // No need to clone extractedPublicKey as it was returned by our native call.
             BoringSSLAsymmetricCipherKeyPair pair =
                     new BoringSSLAsymmetricCipherKeyPair(key, privateKeyBytes.clone(), extractedPublicKey);
-            new EVP_HPKE_KEY_PhantomRef(pair, keyPairRefQueue);
             key = -1;
             return pair;
         } finally {
@@ -284,18 +271,5 @@ public boolean isSupported(KEM kem) {
     public boolean isSupported(KDF kdf) {
         return kdf == KDF.HKDF_SHA256;
     }
-
-    /**
-     * Try to free enqueued {@code EVP_HPKE_KEY}s.
-     */
-    void free_EVP_HPKE_KEYS() {
-        for (;;) {
-            EVP_HPKE_KEY_PhantomRef ref = (EVP_HPKE_KEY_PhantomRef) keyPairRefQueue.poll();
-            if (ref == null) {
-                return;
-            }
-            BoringSSL.EVP_HPKE_KEY_cleanup_and_free(ref.key);
-        }
-    }
 }