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

[Valerie Peng]

"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?

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
>>>