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

[Valerie Peng]

On Wed, 17 Mar 2021 19:34:54 GMT, SalusaSecondus <github.com+829871+SalusaSecondus at openjdk.org> wrote:

>> Valerie Peng has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Changed to use volatile for the default SecureRandom object reference
>
> src/java.base/share/classes/sun/security/jca/JCAUtil.java line 84:
> 
>> 82:      * SecureRandom impl if the provider table is the same.
>> 83:      */
>> 84:     public static SecureRandom getDefSecureRandom() {
> 
> 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?

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.

> src/java.base/share/classes/sun/security/jca/JCAUtil.java line 92:
> 
>> 90:             }
>> 91:         }
>> 92:         return def;
> 
> 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`)
> 
> 
> Suggestion:
> 
>         SecureRandom result = def;
>         if (result == null) {
>             synchronized (JCAUtil.class) {
>                 result = def;
>                 if (result == null) {
>                     def = result = new SecureRandom();
>                 }
>             }
>         }
>         return result;

Sure, good point. I will update. Thanks for catching this.

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

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