Ah yes, you're correct. But because of this delayed selection, the existence of CloneableDelegate is not that useful unless user has specified a provider at the beginning. At first every Signature is a Delegate and thus not a Cloneable, you would have to clone it to make it Cloneable.
I noticed none of our impls are Cloneable (or did I miss one?). What is the motivation to update Signature in this code change? Thanks, Max > On Jun 16, 2020, at 7:29 AM, Valerie Peng <valerie.p...@oracle.com> wrote: > > 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.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
