Code review request, 7115524: sun.security.provider.certpath.ssl.SSLServerCertStore no longer works

[Weijun Wang]

Anyway, I find this a little unsafe. One can add a new SSL security 
provider at run time, and then run this method, and the new provider can 
be loaded.

In my opinion, this is more important than performance.

Thanks
Max


On 11/27/2011 10:15 AM, Xuelei Fan wrote:
>
>
> Sent from my iPad
>
> On Nov 27, 2011, at 9:49 AM, Weijun Wang<weijun.wang at oracle.com>  wrote:
>
>>>> Is it possible that the exception is thrown sometimes but not always?
>>
>> If it always fails, there is no problem failing once at the beginning. Otherwise, if there is a chance it goes fine again after calling some codes, maybe it's better to try call it each time?
>>
> I don't think it the case will happen, otherwise there must be a potential bug. Indeed, the exception  is unlikely to happen once JSSE boot up.
>
> Xuelei
>
>
>> Max
>>
>> On 11/27/2011 09:05 AM, Xuelei Fan wrote:
>>>
>>>
>>> Sent from my iPad
>>>
>>> On Nov 27, 2011, at 12:23 AM, Weijun Wang<weijun.wang at oracle.com>   wrote:
>>>
>>>> There is one thing I'm not sure about:
>>>>
>>>> In which case(s), the GeneralSecurityException will be thrown?
>>> SSLContext initializatio
>>>
>>>> Is it possible that the exception is thrown sometimes but not always? Also, the old code has the exception as a clause of the CertStoreException. Can you also include it?
>>>>
>>> It is reorged in line 100 and 101, the cause is described as unable to initialize the ssl context.
>>>
>>> Xuelei
>>>
>>>> Max
>>>>
>>>>
>>>>
>>>> On Nov 26, 2011, at 11:25 PM, Xuelei Fan<xuelei.fan at oracle.com>   wrote:
>>>>
>>>>> new webrev: http://cr.openjdk.java.net/~xuelei/7115524/webrev.01/
>>>>>
>>>>> You're right on the synchronization issue. In the new webrev, I also
>>>>> remove the "synchronized" keyword on method. Instead, a synchronization
>>>>> on the static attribute is used.
>>>>>
>>>>> Thanks,
>>>>> Xuelei
>>>>>
>>>>> On 11/26/2011 11:16 PM, Weijun Wang wrote:
>>>>>>
>>>>>> On Nov 26, 2011, at 10:58 PM, Xuelei Fan<xuelei.fan at oracle.com>   wrote:
>>>>>>
>>>>>>> I was wondering why we synchronized the getInstance() and
>>>>>>> engineGetCertificates(). It seems that we only need to synchronize the
>>>>>>> serverChain variable.
>>>>>>>
>>>>>>> I will update the synchronization if no objection.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Xuelei
>>>>>>>
>>>>>>> On 11/26/2011 10:42 PM, Xuelei Fan wrote:
>>>>>>>> On 11/26/2011 9:59 PM, Weijun Wang wrote:
>>>>>>>>> Hi Xuelei
>>>>>>>>>
>>>>>>>>> Why move the local variables to static fields?
>>>>>>>>>
>>>>>>>> In order to improve the performance. SSLContext.init() is expensive.
>>>>>>>>
>>>>>>>>> The engineGetCertificates method is only synchronized on this but not the whole class, so there is a chance that the method is called by multiple threads at the same time. This means if you only use one static GetChainTrustManager field, its serverChain field can be modified by different threads.
>>>>>>>>>
>>>>>>>> We have synchronized the engineGetCertificates(), and this method is the
>>>>>>>> only method to access the GetChainTrustManager object. As the attribute
>>>>>>>> is declared as "final static", so it will be initialized once even in
>>>>>>>> multiple threads.
>>>>>>
>>>>>> But this method is only synchronized on the instance, not the class. You can create two SSLServerCertStore objects can call this method at the same time.
>>>>>>
>>>>>> Max
>>>>>>
>>>>>>>>
>>>>>>>> That's, the access to serverChain variable has been synchronized by
>>>>>>>> synchronizing the engineGetCertificates method.
>>>>>>>>
>>>>>>>> Xuelei
>>>>>>>>
>>>>>>>>> -Max
>>>>>>>>>
>>>>>>>>> On Nov 26, 2011, at 8:43 PM, Xuelei Fan wrote:
>>>>>>>>>
>>>>>>>>>> webrev: http://cr.openjdk.java.net/~xuelei/7115524/webrev.00/
>>>>>>>>>>
>>>>>>>>>> No new regression test, the current test,
>>>>>>>>>> test/sun/security/tools/keytool/printssl.sh, has already been able to
>>>>>>>>>> test the update.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Xuelei
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>