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.p...@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.p...@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.p...@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 >>>>>
