RFR: 8260274: Cipher.init(int, key) does not use highest priority provider for random bytes

[SalusaSecondus]

On Mon, 15 Mar 2021 23:31:33 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

>> 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();
>     }

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](https://bugs.openjdk.java.net/browse/JDK-7107615)) creating significant performance issues.

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

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