From b6f8d8dabcffb67d60436783bc8f0f6ea88a2b47 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Tue, 16 Jul 2024 09:13:28 +0200 Subject: [PATCH] Ensure BoringSSLAsymmetricCipherKeyPair will eventually release native 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 --- .../hpke/boringssl/BoringSSLAEADContext.java | 4 +-- .../BoringSSLAsymmetricCipherKeyPair.java | 12 ++++++- .../boringssl/BoringSSLCryptoContext.java | 6 +--- .../hpke/boringssl/BoringSSLHPKEContext.java | 4 +-- .../BoringSSLHPKERecipientContext.java | 4 +-- .../boringssl/BoringSSLHPKESenderContext.java | 4 +-- .../BoringSSLOHttpCryptoProvider.java | 32 ++----------------- 7 files changed, 23 insertions(+), 43 deletions(-) diff --git a/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLAEADContext.java b/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLAEADContext.java index 36acf56..3f695c9 100644 --- a/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLAEADContext.java +++ b/codec-ohttp-hpke-classes-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/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLAsymmetricCipherKeyPair.java b/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLAsymmetricCipherKeyPair.java index 9ff3ed3..f84fed3 100644 --- a/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLAsymmetricCipherKeyPair.java +++ b/codec-ohttp-hpke-classes-boringssl/src/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/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLCryptoContext.java b/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLCryptoContext.java index 0202e17..a85958a 100644 --- a/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLCryptoContext.java +++ b/codec-ohttp-hpke-classes-boringssl/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/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKEContext.java b/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKEContext.java index 8c2e66b..c9373a1 100644 --- a/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKEContext.java +++ b/codec-ohttp-hpke-classes-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/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKERecipientContext.java b/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKERecipientContext.java index 6f069cc..7062052 100644 --- a/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKERecipientContext.java +++ b/codec-ohttp-hpke-classes-boringssl/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/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKESenderContext.java b/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKESenderContext.java index 2af9bc2..404d636 100644 --- a/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLHPKESenderContext.java +++ b/codec-ohttp-hpke-classes-boringssl/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/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLOHttpCryptoProvider.java b/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLOHttpCryptoProvider.java index d79b3ab..1ff6e01 100644 --- a/codec-ohttp-hpke-classes-boringssl/src/main/java/io/netty/incubator/codec/hpke/boringssl/BoringSSLOHttpCryptoProvider.java +++ b/codec-ohttp-hpke-classes-boringssl/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 keyPairRefQueue = new ReferenceQueue<>(); - private static final class EVP_HPKE_KEY_PhantomRef extends PhantomReference { - private final long key; - EVP_HPKE_KEY_PhantomRef(BoringSSLAsymmetricCipherKeyPair referent, - ReferenceQueue 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); - } - } }