[CRYPTO-169][FOLLOWUP] Fix reentrancy of `CryptoRandomFactory#getCryp…

…toRandom` (#259) * fix reentrancy of CryptoRandomFactory#getCryptoRandom * java 8 and 11 * add INIT_ERROR_CLASSES to ReflectionUtils * revert import format * fix import * declare ConcurrentMap * reuse test
apache · Oct 23, 2023 · f0c92d5 · f0c92d5
1 parent a164574
commit f0c92d5
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 11 deletions.
diff --git a/src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java b/src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java
@@ -198,14 +198,6 @@ public static CryptoRandom getCryptoRandom(final Properties props)
             } catch (final Exception e) {
                 lastException = e;
                 errorMessage.append("CryptoRandom: [" + className + "] failed with " + e.getMessage());
-            } catch (final ExceptionInInitializerError initializerError) {
-                Throwable t = initializerError.getCause();
-                if (t instanceof Exception) {
-                    lastException = (Exception) t;
-                    errorMessage.append("CryptoRandom: [" + className + "] initialization failed with " + t.getMessage());
-                } else {
-                    throw initializerError;
-                }
             }
         }
 

diff --git a/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java b/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java
@@ -21,8 +21,12 @@
 import java.lang.reflect.Constructor;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 import java.util.WeakHashMap;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 
 import org.apache.commons.crypto.cipher.CryptoCipher;
 
@@ -40,6 +44,7 @@ private static abstract class AbstractNegativeCacheSentinel {
     }
 
     private static final Map<ClassLoader, Map<String, WeakReference<Class<?>>>> CACHE_CLASSES = new WeakHashMap<>();
+    private static final ConcurrentMap<ClassLoader, Set<String>> INIT_ERROR_CLASSES = new ConcurrentHashMap<>();
 
     private static final ClassLoader CLASSLOADER;
 
@@ -63,6 +68,9 @@ private static abstract class AbstractNegativeCacheSentinel {
     public static Class<?> getClassByName(final String name) throws ClassNotFoundException {
         final Class<?> ret = getClassByNameOrNull(name);
         if (ret == null) {
+            if (INIT_ERROR_CLASSES.get(CLASSLOADER).contains(name)) {
+                throw new IllegalStateException("Class " + name + " initialization error");
+            }
             throw new ClassNotFoundException("Class " + name + " not found");
         }
         return ret;
@@ -73,9 +81,16 @@ public static Class<?> getClassByName(final String name) throws ClassNotFoundExc
      * couldn't be loaded. This is to avoid the overhead of creating an exception.
      *
      * @param name the class name.
-     * @return the class object, or {@code null} if it could not be found.
+     * @return the class object, or {@code null} if it could not be found or initialization failed.
      */
     private static Class<?> getClassByNameOrNull(final String name) {
+        final Set<String> set =
+            INIT_ERROR_CLASSES.computeIfAbsent(CLASSLOADER, k -> Collections.synchronizedSet(new HashSet<>()));
+
+        if (set.contains(name)) {
+            return null;
+        }
+
         final Map<String, WeakReference<Class<?>>> map;
 
         synchronized (CACHE_CLASSES) {
@@ -95,6 +110,10 @@ private static Class<?> getClassByNameOrNull(final String name) {
                 // Leave a marker that the class isn't found
                 map.put(name, new WeakReference<>(NEGATIVE_CACHE_SENTINEL));
                 return null;
+            } catch (final ExceptionInInitializerError error) {
+                // Leave a marker that the class initialization failed
+                set.add(name);
+                return null;
             }
             // two putters can race here, but they'll put the same class
             map.put(name, new WeakReference<>(clazz));

diff --git a/src/test/java/org/apache/commons/crypto/random/CryptoRandomFactoryTest.java b/src/test/java/org/apache/commons/crypto/random/CryptoRandomFactoryTest.java
@@ -82,8 +82,11 @@ public void testExceptionInInitializerErrorRandom() throws GeneralSecurityExcept
         String classes = ExceptionInInitializerErrorRandom.class.getName().concat(",")
             .concat(CryptoRandomFactory.RandomProvider.JAVA.getClassName());
         properties.setProperty(CryptoRandomFactory.CLASSES_KEY, classes);
-        try (final CryptoRandom random = CryptoRandomFactory.getCryptoRandom(properties)) {
-            assertEquals(JavaCryptoRandom.class.getName(), random.getClass().getName());
+        // Invoke 3 times to test the reentrancy of the method in the scenario of class initialization failure.
+        for (int i = 0; i < 3; i++) {
+            try (final CryptoRandom random = CryptoRandomFactory.getCryptoRandom(properties)) {
+                assertEquals(JavaCryptoRandom.class.getName(), random.getClass().getName());
+            }
         }
     }