On Mon, 15 Mar 2021 21:36:08 GMT, Xue-Lei Andrew Fan <[email protected]> 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
