On Mon, 15 Mar 2021 22:11:51 GMT, SalusaSecondus
<github.com+829871+salusasecon...@openjdk.org> wrote:
>> 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.
>
> 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;
Or, I can also separate out the instance returned by getSecureRandom() so that
code path should be unaffected, e.g.:
// 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();
}
-------------
PR: https://git.openjdk.java.net/jdk/pull/3018
