You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2023/10/22 16:41:46 UTC
[PR] [CRYPTO-169][FOLLOWUP] Fix reentrancy of `CryptoRandomFactory#getCryptoRandom` [commons-crypto]
LuciferYang opened a new pull request, #259:
URL: https://github.com/apache/commons-crypto/pull/259
There are some issues with the reentrancy of `CryptoRandomFactory.getCryptoRandom(properties)` method.
For the following test case:
```java
@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());
}
}
```
The second call to `CryptoRandomFactory.getCryptoRandom(properties)` will result in the following error:
```java
java.lang.NoClassDefFoundError: Could not initialize class org.apache.commons.crypto.random.ExceptionInInitializerErrorRandom
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:534)
at java.base/java.lang.Class.forName(Class.java:513)
at org.apache.commons.crypto.utils.ReflectionUtils.getClassByNameOrNull(ReflectionUtils.java:93)
at org.apache.commons.crypto.utils.ReflectionUtils.getClassByName(ReflectionUtils.java:64)
at org.apache.commons.crypto.random.CryptoRandomFactory.getCryptoRandom(CryptoRandomFactory.java:189)
at org.apache.commons.crypto.random.CryptoRandomFactoryTest.testReentrancyOfExceptionInInitializerErrorRandom(CryptoRandomFactoryTest.java:99)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: java.lang.ExceptionInInitializerError: Exception java.lang.IllegalStateException: java.security.GeneralSecurityException: ExceptionInInitializerErrorRandom init failed [in thread "main"]
at org.apache.commons.crypto.random.ExceptionInInitializerErrorRandom.<clinit>(ExceptionInInitializerErrorRandom.java:32)
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:534)
at java.base/java.lang.Class.forName(Class.java:513)
at org.apache.commons.crypto.utils.ReflectionUtils.getClassByNameOrNull(ReflectionUtils.java:93)
at org.apache.commons.crypto.utils.ReflectionUtils.getClassByName(ReflectionUtils.java:64)
at org.apache.commons.crypto.random.CryptoRandomFactory.getCryptoRandom(CryptoRandomFactory.java:189)
at org.apache.commons.crypto.random.CryptoRandomFactoryTest.testReentrancyOfExceptionInInitializerErrorRandom(CryptoRandomFactoryTest.java:96)
... 3 more
```
This is due to the `Class.forName` method. If the first initialization fails, subsequent identical calls will not attempt to initialize the class again, but will throw `NoClassDefFoundError`.
So, this PR provides an additional layer of fault tolerance for ExceptionInInitializerError, which is wrapped by NoClassDefFoundError, and it also includes corresponding test cases to repeatedly test the `CryptoRandomFactory.getCryptoRandom(properties)` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CRYPTO-169][FOLLOWUP] Fix reentrancy of `CryptoRandomFactory#getCryptoRandom` [commons-crypto]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #259:
URL: https://github.com/apache/commons-crypto/pull/259#issuecomment-1774145398
## [Codecov](https://app.codecov.io/gh/apache/commons-crypto/pull/259?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#259](https://app.codecov.io/gh/apache/commons-crypto/pull/259?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (69adb01) into [master](https://app.codecov.io/gh/apache/commons-crypto/commit/a1645747054bb68961791863a9a326260f49a742?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a164574) will **decrease** coverage by `0.14%`.
> The diff coverage is `50.00%`.
```diff
@@ Coverage Diff @@
## master #259 +/- ##
============================================
- Coverage 74.66% 74.53% -0.14%
Complexity 441 441
============================================
Files 38 38
Lines 1816 1826 +10
Branches 177 179 +2
============================================
+ Hits 1356 1361 +5
- Misses 349 352 +3
- Partials 111 113 +2
```
| [Files](https://app.codecov.io/gh/apache/commons-crypto/pull/259?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...che/commons/crypto/random/CryptoRandomFactory.java](https://app.codecov.io/gh/apache/commons-crypto/pull/259?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY3J5cHRvL3JhbmRvbS9DcnlwdG9SYW5kb21GYWN0b3J5LmphdmE=) | `83.33% <50.00%> (-6.67%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CRYPTO-169][FOLLOWUP] Fix reentrancy of `CryptoRandomFactory#getCryptoRandom` [commons-crypto]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #259:
URL: https://github.com/apache/commons-crypto/pull/259#discussion_r1367939088
##########
src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java:
##########
@@ -206,6 +206,18 @@ public static CryptoRandom getCryptoRandom(final Properties props)
} else {
throw initializerError;
}
+ } catch (final NoClassDefFoundError noClassDefFoundError) {
+ Throwable initializerError = noClassDefFoundError.getCause();
Review Comment:
There are some differences here between Java 8/11 and Java 17/21:
- Java 8/11: Will catch `NoClassDefFoundError`, but `NoClassDefFoundError.getCause` is `null`.
- Java 17/21: Will catch `NoClassDefFoundError`, and `NoClassDefFoundError.getCause` is `ExceptionInInitializerError`, but `ExceptionInInitializerError.getCause` is null
So the check way here is a bit tricky, do you have any better suggestions? @garydgregory
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CRYPTO-169][FOLLOWUP] Fix reentrancy of `CryptoRandomFactory#getCryptoRandom` [commons-crypto]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #259:
URL: https://github.com/apache/commons-crypto/pull/259
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CRYPTO-169][FOLLOWUP] Fix reentrancy of `CryptoRandomFactory#getCryptoRandom` [commons-crypto]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #259:
URL: https://github.com/apache/commons-crypto/pull/259#issuecomment-1774989177
Thanks @garydgregory ~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CRYPTO-169][FOLLOWUP] Fix reentrancy of `CryptoRandomFactory#getCryptoRandom` [commons-crypto]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #259:
URL: https://github.com/apache/commons-crypto/pull/259#discussion_r1368086346
##########
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) {
Review Comment:
`ExceptionInInitializerError` is uniformly handled in `ReflectionUtils`, and an exception is thrown at the same time. So, there is no need to handle it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CRYPTO-169][FOLLOWUP] Fix reentrancy of `CryptoRandomFactory#getCryptoRandom` [commons-crypto]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #259:
URL: https://github.com/apache/commons-crypto/pull/259#issuecomment-1774142981
> It seems that there are some differences between Java 8 and 17, let me take another look.
🆗
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CRYPTO-169][FOLLOWUP] Fix reentrancy of `CryptoRandomFactory#getCryptoRandom` [commons-crypto]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #259:
URL: https://github.com/apache/commons-crypto/pull/259#discussion_r1367939088
##########
src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java:
##########
@@ -206,6 +206,18 @@ public static CryptoRandom getCryptoRandom(final Properties props)
} else {
throw initializerError;
}
+ } catch (final NoClassDefFoundError noClassDefFoundError) {
+ Throwable initializerError = noClassDefFoundError.getCause();
Review Comment:
There are some differences here between Java 8/11 and Java 17/21:
- Java 8/11: Here it will catch `NoClassDefFoundError`, but `NoClassDefFoundError.getCause` is `null`.
- Java 17/21: Here it will catch `NoClassDefFoundError`, and `NoClassDefFoundError.getCause` is `ExceptionInInitializerError`, but `ExceptionInInitializerError.getCause` is null
So the check way here is a bit tricky, do you have any better suggestions? @garydgregory
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CRYPTO-169][FOLLOWUP] Fix reentrancy of `CryptoRandomFactory#getCryptoRandom` [commons-crypto]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #259:
URL: https://github.com/apache/commons-crypto/pull/259#issuecomment-1775196320
> Thanks @garydgregory ~
YW!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CRYPTO-169][FOLLOWUP] Fix reentrancy of `CryptoRandomFactory#getCryptoRandom` [commons-crypto]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #259:
URL: https://github.com/apache/commons-crypto/pull/259#discussion_r1367972545
##########
src/main/java/org/apache/commons/crypto/random/CryptoRandomFactory.java:
##########
@@ -206,6 +206,18 @@ public static CryptoRandom getCryptoRandom(final Properties props)
} else {
throw initializerError;
}
+ } catch (final NoClassDefFoundError noClassDefFoundError) {
+ Throwable initializerError = noClassDefFoundError.getCause();
Review Comment:
@LuciferYang
Not off the top of my head...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CRYPTO-169][FOLLOWUP] Fix reentrancy of `CryptoRandomFactory#getCryptoRandom` [commons-crypto]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #259:
URL: https://github.com/apache/commons-crypto/pull/259#discussion_r1368085134
##########
src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java:
##########
@@ -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;
+ }
Review Comment:
Add a collection to record classes that have `ExceptionInInitializerError` to avoid unnecessary reentry of the `Class.forName` method in this scenario. This collection cannot be garbage collected, otherwise there will still be reentry issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CRYPTO-169][FOLLOWUP] Fix reentrancy of `CryptoRandomFactory#getCryptoRandom` [commons-crypto]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #259:
URL: https://github.com/apache/commons-crypto/pull/259#issuecomment-1774141606
It appears that there are some differences between Java 8 and 17, let me take another look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CRYPTO-169][FOLLOWUP] Fix reentrancy of `CryptoRandomFactory#getCryptoRandom` [commons-crypto]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #259:
URL: https://github.com/apache/commons-crypto/pull/259#discussion_r1368085841
##########
src/main/java/org/apache/commons/crypto/utils/ReflectionUtils.java:
##########
@@ -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");
Review Comment:
An additional judgment has been added to distinguish ClassNotFoundException. Here an `IllegalStateException` is manually thrown, there may be a more appropriate exception type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org