From e623f14abcee16b5dfc57d6956e0ab4bb526ba5b Mon Sep 17 00:00:00 2001
From: Alexander Scheel <ascheel@redhat.com>
Date: Wed, 8 Apr 2020 12:21:49 -0400
Subject: [PATCH] Fix NativeProxy registry tracking

When the switch was made to a HashSet-based registry in
eb5df01003d74b57473eacb84e538d31f5bb06ca, NativeProxy didn't override
hashCode(...). This resulted in calls to close() (and thus, finalize())
not invoking the releaseNativeResources() function to release the
underlying memory.

Signed-off-by: Alexander Scheel <ascheel@redhat.com>
---
 org/mozilla/jss/util/NativeProxy.java | 55 +++++++++++++++++++++------
 1 file changed, 44 insertions(+), 11 deletions(-)

diff --git a/org/mozilla/jss/util/NativeProxy.java b/org/mozilla/jss/util/NativeProxy.java
index a0811f76..385c49f9 100644
--- a/org/mozilla/jss/util/NativeProxy.java
+++ b/org/mozilla/jss/util/NativeProxy.java
@@ -9,8 +9,10 @@ import java.util.HashSet;
 import java.lang.AutoCloseable;
 import java.lang.Thread;
 import java.util.Arrays;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import org.mozilla.jss.CryptoManager;
+import org.mozilla.jss.netscape.security.util.Utils;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -39,11 +41,13 @@ public abstract class NativeProxy implements AutoCloseable
      * NativeProxy instance acts as a proxy for that native data structure.
      */
     public NativeProxy(byte[] pointer) {
-		assert(pointer!=null);
+        assert(pointer!=null);
+
         mPointer = pointer;
-        registry.add(this);
+        mHashCode = registryIndex.getAndIncrement();
 
         if (saveStacktraces) {
+            registry.add(this);
             mTrace = Arrays.toString(Thread.currentThread().getStackTrace());
         }
     }
@@ -55,18 +59,31 @@ public abstract class NativeProxy implements AutoCloseable
      *      a different underlying native pointer.
      */
     public boolean equals(Object obj) {
-        if(obj==null) {
+        if (obj == null) {
             return false;
         }
-        if( ! (obj instanceof NativeProxy) ) {
+        if (!(obj instanceof NativeProxy)) {
             return false;
         }
-        if (((NativeProxy)obj).mPointer == null) {
-            /* If mPointer is null, we have no way to compare the values
-             * of the pointers, so assume they're unequal. */
+        NativeProxy nObj = (NativeProxy) obj;
+        if (this.mPointer == null || nObj.mPointer == null) {
             return false;
         }
-        return Arrays.equals(((NativeProxy)obj).mPointer, mPointer);
+
+        return Arrays.equals(this.mPointer, nObj.mPointer);
+    }
+
+    /**
+     * Hash code based around mPointer value.
+     *
+     * Note that Object.hashCode() isn't sufficient as it tries to determine
+     * the Object's value based on all internal variables. Because we want a
+     * single static hashCode that is unique to each instance of nativeProxy,
+     * we construct it up front based on an incrementing counter and cache it
+     * throughout the lifetime of this object.
+     */
+    public int hashCode() {
+        return mHashCode;
     }
 
     /**
@@ -112,11 +129,11 @@ public abstract class NativeProxy implements AutoCloseable
      */
     public final void close() throws Exception {
         try {
-            if (registry.remove(this)) {
+            if (mPointer != null) {
                 releaseNativeResources();
             }
         } finally {
-            mPointer = null;
+            clear();
         }
     }
 
@@ -131,13 +148,16 @@ public abstract class NativeProxy implements AutoCloseable
      */
     public final void clear() {
         this.mPointer = null;
-        registry.remove(this);
+        if (saveStacktraces) {
+            registry.remove(this);
+        }
     }
 
     /**
      * Byte array containing native pointer bytes.
      */
     private byte mPointer[];
+    private int mHashCode;
 
     /**
      * String containing backtrace of pointer generation.
@@ -158,6 +178,15 @@ public abstract class NativeProxy implements AutoCloseable
      * releaseNativeResources() gets called.
      */
     static HashSet<NativeProxy> registry = new HashSet<NativeProxy>();
+    static AtomicInteger registryIndex = new AtomicInteger();
+
+    public String toString() {
+        if (mPointer == null) {
+            return this.getClass().getName() + "[" + mHashCode + "@null]";
+        }
+
+        return this.getClass().getName() + "[" + mHashCode + "@" + Utils.HexEncode(mPointer) + "]";
+    }
 
     /**
      * Internal helper to check whether or not assertions are enabled in the
@@ -178,6 +207,10 @@ public abstract class NativeProxy implements AutoCloseable
      * is thrown.
      */
     public synchronized static void assertRegistryEmpty() {
+        if (!saveStacktraces) {
+            return;
+        }
+
         if (!registry.isEmpty()) {
             logger.warn(registry.size() + " NativeProxys are still registered.");
 
-- 
2.25.2