On Mon, 15 Mar 2021 21:36:08 GMT, Xue-Lei Andrew Fan <xue...@openjdk.org> wrote:

>> 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?
>
> 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;

-------------

PR: https://git.openjdk.java.net/jdk/pull/3018

Reply via email to