From 65f5e66ad888e0a495169257c3ea901f9d54550f Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Mon, 23 Oct 2023 00:33:20 +0800 Subject: [PATCH 1/7] fix reentrancy of CryptoRandomFactory#getCryptoRandom --- .../crypto/random/CryptoRandomFactory.java | 8 ++++++++ .../crypto/random/CryptoRandomFactoryTest.java | 17 +++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java b/src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java index 8239ae0a7..0f3c273f8 100644 --- a/src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java +++ b/src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java @@ -206,6 +206,14 @@ public static CryptoRandom getCryptoRandom(final Properties props) } else { throw initializerError; } + } catch (final NoClassDefFoundError noClassDefFoundError) { + Throwable initializerError = noClassDefFoundError.getCause(); + if (initializerError instanceof ExceptionInInitializerError) { + lastException = new IllegalStateException(initializerError.getMessage()); + errorMessage.append("CryptoRandom: [" + className + "] initialization failed with " + initializerError.getMessage()); + } else { + throw noClassDefFoundError; + } } } diff --git a/src/test/java/org/apache/commons/crypto/random/CryptoRandomFactoryTest.java b/src/test/java/org/apache/commons/crypto/random/CryptoRandomFactoryTest.java index 4e2050222..cbd1c4f05 100644 --- a/src/test/java/org/apache/commons/crypto/random/CryptoRandomFactoryTest.java +++ b/src/test/java/org/apache/commons/crypto/random/CryptoRandomFactoryTest.java @@ -87,6 +87,23 @@ public void testExceptionInInitializerErrorRandom() throws GeneralSecurityExcept } } + @Test + public void testReentrancyOfExceptionInInitializerErrorRandom() throws GeneralSecurityException, IOException { + final Properties properties = new Properties(); + 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()); + } + try (final CryptoRandom random = CryptoRandomFactory.getCryptoRandom(properties)) { + assertEquals(JavaCryptoRandom.class.getName(), random.getClass().getName()); + } + try (final CryptoRandom random = CryptoRandomFactory.getCryptoRandom(properties)) { + assertEquals(JavaCryptoRandom.class.getName(), random.getClass().getName()); + } + } + @Test public void testFailingRandom() { final Properties properties = new Properties(); From 69adb018951d476fb39a4cc2745c5d4790ad02ae Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Mon, 23 Oct 2023 01:00:51 +0800 Subject: [PATCH 2/7] java 8 and 11 --- .../apache/commons/crypto/random/CryptoRandomFactory.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java b/src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java index 0f3c273f8..5bb584e5e 100644 --- a/src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java +++ b/src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java @@ -208,9 +208,13 @@ public static CryptoRandom getCryptoRandom(final Properties props) } } catch (final NoClassDefFoundError noClassDefFoundError) { Throwable initializerError = noClassDefFoundError.getCause(); + String message = noClassDefFoundError.getMessage(); if (initializerError instanceof ExceptionInInitializerError) { - lastException = new IllegalStateException(initializerError.getMessage()); + lastException = new IllegalStateException(initializerError.getMessage()); errorMessage.append("CryptoRandom: [" + className + "] initialization failed with " + initializerError.getMessage()); + } else if (initializerError == null && message != null && message.startsWith("Could not initialize class")) { + lastException = new IllegalStateException(message); + errorMessage.append("CryptoRandom: [" + className + "] initialization failed with " + message); } else { throw noClassDefFoundError; } From 29425230c9ec872fd7ee19d8dfa3f58c27a7dbab Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Mon, 23 Oct 2023 09:10:31 +0800 Subject: [PATCH 3/7] add INIT_ERROR_CLASSES to ReflectionUtils --- .../crypto/random/CryptoRandomFactory.java | 20 ---------------- .../commons/crypto/utils/ReflectionUtils.java | 23 +++++++++++++++---- .../random/CryptoRandomFactoryTest.java | 12 ++++------ 3 files changed, 22 insertions(+), 33 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 index 5bb584e5e..8acdf9c8d 100644 --- a/src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java +++ b/src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java @@ -198,26 +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; - } - } catch (final NoClassDefFoundError noClassDefFoundError) { - Throwable initializerError = noClassDefFoundError.getCause(); - String message = noClassDefFoundError.getMessage(); - if (initializerError instanceof ExceptionInInitializerError) { - lastException = new IllegalStateException(initializerError.getMessage()); - errorMessage.append("CryptoRandom: [" + className + "] initialization failed with " + initializerError.getMessage()); - } else if (initializerError == null && message != null && message.startsWith("Could not initialize class")) { - lastException = new IllegalStateException(message); - errorMessage.append("CryptoRandom: [" + className + "] initialization failed with " + message); - } else { - throw noClassDefFoundError; - } } } diff --git a/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java b/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java index f5fa8cae0..dc857b5c0 100644 --- a/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java +++ b/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java @@ -19,10 +19,8 @@ import java.lang.ref.WeakReference; import java.lang.reflect.Constructor; -import java.util.Arrays; -import java.util.Collections; -import java.util.Map; -import java.util.WeakHashMap; +import java.util.*; +import java.util.concurrent.ConcurrentHashMap; import org.apache.commons.crypto.cipher.CryptoCipher; @@ -40,6 +38,7 @@ private static abstract class AbstractNegativeCacheSentinel { } private static final Map>>> CACHE_CLASSES = new WeakHashMap<>(); + private static final Map> INIT_ERROR_CLASSES = new ConcurrentHashMap<>(); private static final ClassLoader CLASSLOADER; @@ -63,6 +62,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 +75,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 set = + INIT_ERROR_CLASSES.computeIfAbsent(CLASSLOADER, k -> Collections.synchronizedSet(new HashSet<>())); + + if (set.contains(name)) { + return null; + } + final Map>> map; synchronized (CACHE_CLASSES) { @@ -95,6 +104,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 index cbd1c4f05..827f1cf74 100644 --- a/src/test/java/org/apache/commons/crypto/random/CryptoRandomFactoryTest.java +++ b/src/test/java/org/apache/commons/crypto/random/CryptoRandomFactoryTest.java @@ -93,14 +93,10 @@ public void testReentrancyOfExceptionInInitializerErrorRandom() throws GeneralSe 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()); - } - try (final CryptoRandom random = CryptoRandomFactory.getCryptoRandom(properties)) { - assertEquals(JavaCryptoRandom.class.getName(), random.getClass().getName()); - } - try (final CryptoRandom random = CryptoRandomFactory.getCryptoRandom(properties)) { - assertEquals(JavaCryptoRandom.class.getName(), random.getClass().getName()); + for (int i = 0; i < 3; i++) { + try (final CryptoRandom random = CryptoRandomFactory.getCryptoRandom(properties)) { + assertEquals(JavaCryptoRandom.class.getName(), random.getClass().getName()); + } } } From d90ce350310817b104e4aaec23520d3c6de3b46b Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Mon, 23 Oct 2023 09:12:10 +0800 Subject: [PATCH 4/7] revert import format --- .../org/apache/commons/crypto/utils/ReflectionUtils.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java b/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java index dc857b5c0..46575c2c9 100644 --- a/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java +++ b/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java @@ -19,7 +19,10 @@ import java.lang.ref.WeakReference; import java.lang.reflect.Constructor; -import java.util.*; +import java.util.Arrays; +import java.util.Collections; +import java.util.Map; +import java.util.WeakHashMap; import java.util.concurrent.ConcurrentHashMap; import org.apache.commons.crypto.cipher.CryptoCipher; From 8fe6742168c7b73eeac177c0919764d34865dfa9 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Mon, 23 Oct 2023 09:14:05 +0800 Subject: [PATCH 5/7] fix import --- .../java/org/apache/commons/crypto/utils/ReflectionUtils.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java b/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java index 46575c2c9..91395c354 100644 --- a/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java +++ b/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java @@ -21,7 +21,9 @@ 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; From 01a49e86072904505172d2747905de86a07f8b5d Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Mon, 23 Oct 2023 11:03:17 +0800 Subject: [PATCH 6/7] declare ConcurrentMap --- .../java/org/apache/commons/crypto/utils/ReflectionUtils.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java b/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java index 91395c354..e941dee7b 100644 --- a/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java +++ b/src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java @@ -26,6 +26,7 @@ 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; @@ -43,7 +44,7 @@ private static abstract class AbstractNegativeCacheSentinel { } private static final Map>>> CACHE_CLASSES = new WeakHashMap<>(); - private static final Map> INIT_ERROR_CLASSES = new ConcurrentHashMap<>(); + private static final ConcurrentMap> INIT_ERROR_CLASSES = new ConcurrentHashMap<>(); private static final ClassLoader CLASSLOADER; From fa434ec0e43da4586bf66a6e9cc19ab3c52e306e Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Mon, 23 Oct 2023 11:15:31 +0800 Subject: [PATCH 7/7] reuse test --- .../crypto/random/CryptoRandomFactoryTest.java | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/test/java/org/apache/commons/crypto/random/CryptoRandomFactoryTest.java b/src/test/java/org/apache/commons/crypto/random/CryptoRandomFactoryTest.java index 827f1cf74..111948312 100644 --- a/src/test/java/org/apache/commons/crypto/random/CryptoRandomFactoryTest.java +++ b/src/test/java/org/apache/commons/crypto/random/CryptoRandomFactoryTest.java @@ -82,17 +82,7 @@ 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()); - } - } - - @Test - public void testReentrancyOfExceptionInInitializerErrorRandom() throws GeneralSecurityException, IOException { - final Properties properties = new Properties(); - String classes = ExceptionInInitializerErrorRandom.class.getName().concat(",") - .concat(CryptoRandomFactory.RandomProvider.JAVA.getClassName()); - properties.setProperty(CryptoRandomFactory.CLASSES_KEY, classes); + // 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());