Code Review Request JDK-8129988 JSSE should create a single instance of the cacerts KeyStore

Xuelei Fan at
Wed Jan 4 18:38:06 UTC 2017

Updated to use synchronized method.  Most of the time, the synchronized 
only perform the read access to the variables. The impact on performance 
should be acceptable.

Only the implementation get updated in this webrev.


On 12/23/2016 7:52 AM, Sean Mullan wrote:
> On 12/22/16 2:52 PM, Xuelei Fan wrote:
>> updated:
> I think there are still some race conditions. For example:
>  264             TrustStoreDescriptor temporaryDesc = this.descriptor;
>  265             KeyStore cachedKeyStore = ksRef.get();
>  266             if (descriptor.equals(temporaryDesc) && (cachedKeyStore
> != null)) {
>  267                 return cachedKeyStore;
>  268             }
> There is no locking here.
> Maybe that's ok based on your explanation below, but it seems a bit
> fragile and could lead to problems that are hard to debug. Have you
> looked at the AtomicReference class? You could define a new class
> containing the descriptor, keystore, and certs and wrap that in an
> AtomicReference and then use the methods on that class to update it.
> Might be worth exploring that a bit more.
> --Sean
>> On 12/22/2016 9:32 AM, Sean Mullan wrote:
>>> On 12/20/16 3:21 PM, Xuelei Fan wrote:
>>>>>  213             if (storePassword != null &&
>>>>> !storePassword.isEmpty()) {
>>>>>  214                 MessageDigest md =
>>>>> JsseJce.getMessageDigest("SHA-256");
>>>>>  215                 result = 31 * result +
>>>>>  216 Arrays.hashCode(md.digest(storePassword.getBytes()));
>>>>>  217             }
>>>>> Why are you hashing the password here? Are you afraid this could be
>>>>> leaked or guessed somehow?
>>>> Yes.  The hash code of the password part can be computed.  I was
>>>> wondering the String.hashCode() may not have sufficient strength.
>>>>> I would just leave the password out of the
>>>>> hashcode and equals. It doesn't matter, it's still the same file,
>>>>> right?
>>>>> I'm not sure if the type or provider matter either. Don't you just
>>>>> care
>>>>> about the name of the file and the modification time?
>>>> For file type key store, the file and the modification time should be
>>>> sufficient.  But for non-file (PKCS11) key store, the provider and
>>>> password may be sensible.  The basic idea is that, if one of the system
>>>> property get updated, the key store should be reloaded.  Checking every
>>>> property update makes the code more straightforward.
>>> But the main focus of this performance issue is for the cacerts file,
>>> which is not PKCS11. So I would not use the password and other
>>> non-relevant or security-sensitive attributes. A hash of the password
>>> isn't sufficient against dictionary-type attacks, for example.
>> I see your point.  The password hash code block is removed.
>>>>>  268             if ((temporaryDesc != null) &&
>>>>> Why would a null descriptor ever be ok? Shouldn't you just let this
>>>>> throw NPE? Same comment on line 301.
>>>> The temporaryDesc is initialized as null.  A singleton service
>>>> (TrustStoreManage.tam) is used and lazy loaded.  Null means the
>>>> descriptor has not been assigned.
>>> I think there are thread-safeness issues in the TrustStoreManager class.
>>> You are not synchronizing when you read so looks like there can be
>>> various race conditions. For example, this.descriptor and this.ksRef can
>>> be updated by another thread in the middle of this code
>>>  267             TrustStoreDescriptor temporaryDesc = this.descriptor;
>>>  268             if ((temporaryDesc != null) &&
>>>  269                 temporaryDesc.equals(descriptor)) {
>>>  270                 KeyStore ks = ksRef.get();
>>>  271                 if (ks != null) {
>>>  272                     return ks;
>>>  273                 }
>>>  274             }
>>> Maybe that doesn't really matter, but I'm not sure -- have you thought
>>> about it?
>> I thought about the issue.  But I really missed to the double check
>> idiom.  Updated.
>> For performance consideration, I'm trying to mitigate the impact of
>> synchronization.  Once the key store get loaded, there is a strong
>> reference, and it can be used safely.  If another thread is trying to
>> modify the descriptor and key store, this thread will use the existing
>> key store, and another thread can use the new key store.  If two threads
>> try to modify the key store for the same descriptor, I added the double
>> check idiom so that the first thread will complete the update and the
>> 2nd thread will use the 1st thread updated key store.  If two threads
>> try to modify the key store for different descriptor, each will get a
>> different key store and the 2nd thread will reset the final key store
>> for future use.
>> In general, applications would not modify the system properties.  So the
>> use of the synchronized block should be very rare.  It benefits the
>> performance in multiple threading computation environment.
>> Xuelei
>>> --Sean

More information about the security-dev mailing list