[15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable

[Xuelei Fan]

On 6/8/2020 1:42 PM, Valerie Peng wrote:
> "md instanceof Cloneable && md is not from PKCS11" is not entirely 
> precise. What I mean is that for MessageDigestSpi impls from PKCS11 
> provider, we will need to call the clone() to know for sure whether 
> cloning is supported. If we decide to employ these extra logic for 
> saving clone() call, it's better to do it inside the 
> MessageDigest.of(...) so the callers don't have to repeat the same 
> logic. Comments?
> 
It sounds like a good idea to me if the repeatedly checking could be avoid.

Xuelei

> Valerie
> 
> On 6/8/2020 1:34 PM, Valerie Peng wrote:
>> Hmm, I checked all MessageDigestSpi impls of JDK providers... The 
>> story is more complicated than we expect.
>>
>> For SUN provider which implement the digest spi, implementing 
>> Cloneable means it supports clone functionality.
>>
>> However, for SunPKCS11 provider which wraps native PKCS11 library, it 
>> always implements Cloneable interface, but when clone() is called, it 
>> will then perform the equivalent PKCS11 calls and throw CNSE if any 
>> PKCS11 error is observed.
>>
>> So, there is a possibility that the instanceof check and the clone() 
>> check leads to different result in this particular scenario.
>>
>> The chance of 3rd non-PKCS11 party provider whose 
>> MessageDigest/MessageDigestSpi impl implements Cloneable but throws 
>> CNSE when clone() is called should be very low? So, I think it should 
>> be sufficient to use "md instanceof Cloneable && md is not from PKCS11"?
>>
>> Valerie
>>
>> On 6/6/2020 9:10 AM, Xuelei Fan wrote:
>>> As the the Delegate class takes care of the Cloneable SPI 
>>> implementation, it should be sufficient to use "md instanceof 
>>> Cloneable" only.  It is not a expected behavior that a provider 
>>> implements Cloneable but does not really support it.
>>>
>>> Xuelei
>>>
>>> On 6/5/2020 10:54 PM, Weijun Wang wrote:
>>>> Is it possible to try "md instanceof Cloneable || md.clone() 
>>>> returns"? Hopefully this is fast enough in most cases and also has 
>>>> the chance to recognize more actually-cloneable ones.
>>>>
>>>> I'm also OK with simply using "md instanceof Cloneable".
>>>>
>>>> --Max
>>>>
>>>>> On Jun 6, 2020, at 12:02 PM, Valerie Peng <valerie.peng at oracle.com> 
>>>>> wrote:
>>>>>
>>>>>
>>>>> I am merely following the spec's recommendation of trying the 
>>>>> clone() for cloneability check.
>>>>>
>>>>> If you both are ok with it and prefer the instanceof check, I can 
>>>>> sure reverting back the changes in HmacCore and HandshakeHash classes.
>>>>>
>>>>> Valerie
>>>>>
>>>>> On 6/5/2020 2:04 AM, Seán Coffey wrote:
>>>>>> I share the same concern. clone() is a heavy weight operation in 
>>>>>> constructors that can be called alot during intensive crypto 
>>>>>> operations.
>>>>>>
>>>>>> Now that you've done work on Delegate class - why not also keep 
>>>>>> the (instanceof Cloneable) test ? That can serve as your fastpath 
>>>>>> for the default JDK configuration AFAIK.
>>>>>>
>>>>>> regards,
>>>>>> Sean.
>>>>>>
>>>>>> On 05/06/2020 00:16, Weijun Wang wrote:
>>>>>>>
>>>>>>>> 在 2020年6月5日，03:19，Valerie Peng <valerie.peng at oracle.com> 
>>>>>>>> 写道：
>>>>>>>>
>>>>>>>>> Can you give an example when these 2 rules have different 
>>>>>>>>> results? Is this only true for those implementation that 
>>>>>>>>> directly subclass MessageDigest?
>>>>>>>> Before this fix, even the Spi impl implements Cloneable the 
>>>>>>>> instanceof check will always fail because the wrapper class, 
>>>>>>>> i.e. MessageDigest.Delegate does not. However, if you call the 
>>>>>>>> clone() (made public by the MessageDigest class), it will 
>>>>>>>> succeed because Delegate.clone() checks to see if the spi object 
>>>>>>>> implements the Cloneable interface, if yes, it will proceed to 
>>>>>>>> call the spi clone(). So, for this scenario, the results are 
>>>>>>>> different, e.g. instanceof returns false, but clone() succeeds. 
>>>>>>>> This is just one example. Is this what you are asking?
>>>>>>> No.
>>>>>>>
>>>>>>> I understand this case, but this has already been fixed. Is there 
>>>>>>> any other example? Or are you only follow the words in the spec? 
>>>>>>> i.e. try clone() to see if it’s cloneable.
>>>>>>>
>>>>>>> I am worried that try clone() is much heavier than just check 
>>>>>>> instanof Cloneable.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Max
>>>>