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

[Valerie Peng]

Hi Max,

Please find replies in line.

On 6/4/2020 3:54 AM, Weijun Wang wrote:
> HmacCore.java:
>
>    78                 md = null;
>    79                 String noCloneProv = md.getProvider().getName();
>
> NPE on line 79. Should reverse.
Good catch, fixed.
>> On Jun 4, 2020, at 8:09 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>
>> Hi,
>>
>> Anyone can help review this fix? I changed com.sun.crypto.provider.HmacCore class and sun.security.ssl.HandshakeHash class to check for cloneability based on the clone() call instead of the Cloneable interface.
> 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?

> The test is not complete. There should be non-cloneable hash. Or even better, a hash which is not cloneable from the preferred provider but cloneable from another. Hopefully you can simply reuse an existing implementation with a different algorithm name and override its clone() to throw CNSE.

Right, I will expand the tests. Just want to gauge on people's 
preference of matching the Cloneable interface (the last part of changes 
in my earlier email) first, while I explore the existing tests.

Thanks,

Valerie

>
> Thanks,
> Max
>
>
>> Noticed a bug in sun.security.provider.DigestBase class which misses to reset the temporary buffer when cloning and fixed it here as well.
>>
>> Lastly, I also made changes to java.security.MessageDigest and java.security.Signature classes and attempt to match the Delegate wrapper with the underlying spi object, i.e. if the spi implements Cloneable and then Delegate wrapper also implements Cloneable. This part is mostly for non-JDK callers which rely on the instanceof Cloneable check for cloneability. However, for Signature class, if the object is requested using Signature.getInstance(String), then we can't do matching here since the underlying spi is not yet determined at this time. The 3  TestCloneable.java tests are for testing this last part and is NOT for the HmacCore and HandshakeHash changes. I am on the fence about this part and am open to leave this out if minimum fix is preferred.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8246077
>> Webrev: http://cr.openjdk.java.net/~valeriep/8246077/webrev.00/
>>
>> Thanks,
>>
>> Valerie
>>