8260274: Cipher.init(int, key) does not use highest priority provider for random bytes #3018
Conversation
… for random bytes
|
👋 Welcome back valeriep! A progress list of the required criteria for merging this PR into |
|
@valeriepeng The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| private static Provider[] cachedConfig = Security.getProviders(); | ||
| private static SecureRandom instance = new SecureRandom(); | ||
| public static SecureRandom instance(boolean checkConfig) { | ||
| synchronized (CachedSecureRandomHolder.class) { |
There was a problem hiding this comment.
Is there a performance regression because of this synchronization?
There was a problem hiding this comment.
It depends, this may not be a "hot area" where there is a lot of contention? Or do you feel maybe we should just go with the slower "new SecureRandom()" call for each affected class? I am on the fence actually.
There was a problem hiding this comment.
Since 5 classes have init() calls that would be acquiring this lock, something that could be independent would be better. Calling "new SecureRandom()" may or may not be better, but hope it wouldn't lock over operations during creation.
I'm just throwing out an idea:
Can the provider index number be verify in the list each time?
Better yet, maybe a way to have this triggered each time a provider is added or removed from the list?
There was a problem hiding this comment.
Cipher and KeyPairGenerator could be used a lot in TLS context. The synchronization may impact in some circumstances. Supporting dynamic configuration could be error-prone. For example, one thread to set the providers, and another thread get the providers by calling Cipher.init(). Unless the set and get use the same lock, the return value of the get method is not predictable. The lock here is on CachedSecureRandomHolder for providers configuration getting, and no lock (or different lock) for providers configuration setting. So applications may be able to get the expected behaviors by introducing this synchronization.
As this behavior has been a while, I may not update the implementation. Instead, I would like to add an "implNote" tag to state that once the default provider is loaded, an implementation may be changed it any longer. Applications could use provider parameter if there is a preference.
If you want go ahead, maybe the performance impacted could be mitigated by locking less processes. For example:
if (checkConfig) {
Provider[] currConfig = Security.getProviders();
if (!Arrays.equals(cachedConfig, currConfig)) {
synchronized (CachedSecureRandomHolder.class) {
// double check the currConfig
currConfig = Security.getProviders();
if (!Arrays.equals(cachedConfig, currConfig)) {
instance = ...
cachedConfig = ...
}
}
}
}
return instance;
However, I still not very sure of the performance of Security.getProviders() because it is synchronized as well. Further, I may want to evaluate if the update of Security provider (for example Providers.setProviderList) could trigger an even to update the default instances.
There was a problem hiding this comment.
I would definitely not call new SecureRandom() each time as that can have real performance problems (especially on systems which pull from /dev/random and may not have hardware entropy). The synchronization worries me too.
Could java.security.Security contain a "provider version" int (volatile, atomic, or similar) which is incremented every time the provider list is changed? Then this class (clearly using some non-public API) would check that value and if it is the same, just return the cached instance. If it has changed since the last time, then it would update instance and its remembered value of the index. I think that we might be able to do it without taking any locks in this class because even if the providers change out from under us, we returned a reasonable value (subject to multi-threading) and will fix things up when we're called the next time.
// using this and name collision to highlight what's a field and what's local
int providerListVersion = Security.nonPublicGetProviderListVersion();
if (this.providerListVersion != providerListVersion) {
this.providerListVersion = providerListVersion;
this.instance = new SecureRandom();
}
return this.instance;There was a problem hiding this comment.
Doesn't that still incur a JVM-wide lock whenever CachedSecureRandomHolder.getDefSecureRandom() is called? I recall a different JVM-wide lock in the getInstance path (JDK-7107615) creating significant performance issues.
There was a problem hiding this comment.
I did not get the point that the code path should be unaffected". As there are a few update like:
init(keysize, JCAUtil.getSecureRandom());
init(keysize, JCAUtil.getDefSecureRandom());
I think there are some impact on the existing application which use the default sec
Or, I can also separate out the instance returned by getSecureRandom() so that code path should be unaffected, e.g.:
I did not get the point that the "code path should be unaffected"? JCAUtil.getDefSecureRandom() is used in updates like:
init(keysize, JCAUtil.getSecureRandom());
init(keysize, JCAUtil.getDefSecureRandom());
// cached SecureRandom instance private static class CachedSecureRandomHolder { public static SecureRandom instance = new SecureRandom(); private static Provider[] cachedConfig = Security.getProviders(); private static SecureRandom def = instance; public static SecureRandom getDefault() { synchronized (CachedSecureRandomHolder.class) { Provider[] currConfig = Security.getProviders(); if (!Arrays.equals(cachedConfig, currConfig)) { def = new SecureRandom(); cachedConfig = currConfig; } } return def; } } /** * Get a SecureRandom instance. This method should be used by JDK * internal code in favor of calling "new SecureRandom()". That needs to * iterate through the provider table to find the default SecureRandom * implementation, which is fairly inefficient. */ public static SecureRandom getSecureRandom() { return CachedSecureRandomHolder.instance; } /** * Get the default SecureRandom instance. This method is the * optimized version of "new SecureRandom()" which re-uses the default * SecureRandom impl if the provider table is the same. */ public static SecureRandom getDefSecureRandom() { return CachedSecureRandomHolder.getDefault(); }
There was a problem hiding this comment.
As this behavior has been a while, I may not update the implementation. Instead, I would like to add an "implNote" tag to state that once the default provider is loaded, an implementation may be changed it any longer. Applications could use provider parameter if there is a preference.
Well, javadoc of the affected init methods clearly specifies that "get them using the SecureRandom implementation of the highest-priority installed provider as the source of randomness. ", thus I am not sure if we can just get around this with an @implNote.
There was a problem hiding this comment.
I did not get the point that the code path should be unaffected". As there are a few update like:
init(keysize, JCAUtil.getSecureRandom());
init(keysize, JCAUtil.getDefSecureRandom());
What I mean is that the new suggestion will not impact callers of JCAUtil.getSecureRandom() which I thought what your performance regression means. To enforce that the most preferred SecureRandom impl is used, this for sure will incur some cost especially when the existing impl just returns a cached obj w/o any checking.
There was a problem hiding this comment.
Could
java.security.Securitycontain a "provider version"int(volatile, atomic, or similar) which is incremented every time the provider list is changed? Then this class (clearly using some non-public API) would check that value and if it is the same, just return the cached instance. If it has changed since the last time, then it would updateinstanceand its remembered value of the index. I think that we might be able to do it without taking any locks in this class because even if the providers change out from under us, we returned a reasonable value (subject to multi-threading) and will fix things up when we're called the next time.
This and Tony's suggestion sounds similar and I can sure explore this.
|
From a performance standpoint, having a single synchronized block on every init() operations is going to slow things down |
| @@ -26,7 +26,7 @@ | |||
| package sun.security.jca; | |||
|
|
|||
| import java.lang.ref.*; | |||
|
|
|||
| import java.util.Arrays; | |||
There was a problem hiding this comment.
Import no longer needed.
| * optimized version of "new SecureRandom()" which re-uses the default | ||
| * SecureRandom impl if the provider table is the same. | ||
| */ | ||
| public static SecureRandom getDefSecureRandom() { |
There was a problem hiding this comment.
When should people use getSecureRandom rather than getDefSecureRandom()? Are we leaving the old to avoid changing behavior out from under people who may not expect it?
There was a problem hiding this comment.
To minimize the impact, I leave the JCAUtil.getSecureRandom() impl as is. I suppose this is more for JDK internal code which is not required to use the most preferred SecureRandom impl. There are quite a few callers to this method and I feel it's better to leave them out of this change.
There was a problem hiding this comment.
To minimize the impact, I leave the JCAUtil.getSecureRandom() impl as is. I suppose this is more for JDK internal code which is not required to use the most preferred SecureRandom impl. There are quite a few callers to this method and I feel it's better to leave them out of this change.
The internal use may be just to avoid to create new instances of SecureRandom. Those use may need to use the latest updated secure random provider as well, for example the use in DSA key pair generation (DSAKeyPairGenerator.java).
With this update, I think the performance impact should be minimal and we may be able to have a uniform behavior no matter internal or external uses.
There was a problem hiding this comment.
Maybe. We can do that in a separate changeset...
| if (def == null) { | ||
| synchronized (JCAUtil.class) { | ||
| if (def == null) { | ||
| def = new SecureRandom(); | ||
| } | ||
| } | ||
| } | ||
| return def; |
There was a problem hiding this comment.
Do we need to worry about clearDefSecureRandom() being called between lines 88 and 92?
Thread 1: getDefSecureRandom()#85 (def is null)
Thread 1: getDefSecureRandom()#86
Thread 1: getDefSecureRandom()#87 (def is still null)
Thread 1: getDefSecureRandom()#88 (def is not null)
Thread 2: clearDefSecureRandom() (def is null again)
Thread 1: getDefSecureRandom()#92 (return def which is incorrectly null)
| if (def == null) { | |
| synchronized (JCAUtil.class) { | |
| if (def == null) { | |
| def = new SecureRandom(); | |
| } | |
| } | |
| } | |
| return def; | |
| SecureRandom result = def; | |
| if (result == null) { | |
| synchronized (JCAUtil.class) { | |
| result = def; | |
| if (result == null) { | |
| def = result = new SecureRandom(); | |
| } | |
| } | |
| } | |
| return result; |
There was a problem hiding this comment.
Sure, good point. I will update. Thanks for catching this.
| public static SecureRandom getDefSecureRandom() { | ||
| SecureRandom result = def; | ||
| if (result == null) { | ||
| synchronized (JCAUtil.class) { |
There was a problem hiding this comment.
Could this lock be avoided if set the value in the Providers update (or when the providers list is updated)?
There was a problem hiding this comment.
Well, then we have to pay the cost of "new SecureRandom()" at every provider list update when it may not be needed. It's hard to say which way is better... I thought about it and it seems more reasonable to only pay the cost when the SecureRandom object is needed.
|
I'm fine with the change. The main code path should perform as it did previous to the change and I don't think the lock here is a big deal as it will only be triggered during a provider change. One small delay in a very rare case should be fine |
|
@valeriepeng This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 82 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
Thanks all for the review and feedback~ |
|
/integrate |
openjdk
bot
added
integrated
|
@valeriepeng Since your change was applied there have been 103 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 434a399. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Can someone help review this somewhat trivial change?
Updated JCAUtil class to return the cached SecureRandom object only when the provider configuration has not changed.
Added a regression test to check the affected classes, i.e. AlgorithmParameterGenerator, KeyPairGenerator, Cipher, KeyAgreement, KeyGenerator.
Thanks,
Valerie
Progress
Issue
Reviewers
Download
To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3018/head:pull/3018$ git checkout pull/3018To update a local copy of the PR:
$ git checkout pull/3018$ git pull https://git.openjdk.java.net/jdk pull/3018/head