On Wed, 17 Mar 2021 19:34:54 GMT, SalusaSecondus
<github.com+829871+salusasecon...@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
