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

[Valerie Peng]

Hmm, on a second thought, I reverted back on this last suggestion. 
Signature class has this delayed provider selection mechanism, so the 
clone() method should always rely on the chosen signatureSpi obj.

Thanks,
Valerie
On 6/15/2020 12:59 PM, Valerie Peng wrote:
> Sure, sounds good. Webrev is updated in place at webrev.01 since the 
> change is just one-line.
>
> Will proceed with integration once the mach5 tests finish.
>
> Thanks!
> Valerie
> On 6/14/2020 2:21 AM, Weijun Wang wrote:
>> Looks fine to me. Maybe you can also use "if (this instanceof 
>> Cloneable)" in Signature$Delegate::clone.
>>
>> Thanks,
>> Max
>>
>>> On Jun 11, 2020, at 3:45 AM, Valerie Peng <valerie.peng at oracle.com> 
>>> wrote:
>>>
>>> Webrev updated at: 
>>> http://cr.openjdk.java.net/~valeriep/8246077/webrev.01/
>>>
>>> Key changes in this revision are in the Delegate.of() method in 
>>> java.security.MessageDigest class.
>>>
>>> Comments?
>>>
>>> Thanks,
>>> Valerie
>>> 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?
>>>>
>>>> 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