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

2020-06-16 Thread Valerie Peng
Right, I debated about that myself - the changes to the Signature class is more for 3rd party impls and apps which may support Cloning and rely on instanceof check. Made the changes as it's harmless at least. The javadoc wording regarding cloning in MessageDigest class seems more needed for Si

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

2020-06-15 Thread Weijun Wang
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 notice

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

2020-06-15 Thread 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 upda

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

2020-06-15 Thread Valerie Peng
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 Signat

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

2020-06-14 Thread Weijun Wang
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 wrote: > > Webrev updated at: http://cr.openjdk.java.net/~valeriep/8246077/webrev.01/ > > Key changes in this revision are in the Del

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

2020-06-10 Thread Valerie Peng
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" i

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

2020-06-08 Thread Xuelei Fan
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

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

2020-06-08 Thread 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

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

2020-06-08 Thread Valerie Peng
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 alw

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

2020-06-06 Thread Xuelei Fan
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 p

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

2020-06-05 Thread Weijun Wang
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 wrot

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

2020-06-05 Thread Valerie Peng
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 th

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

2020-06-05 Thread Seán Coffey
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 config

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

2020-06-04 Thread Weijun Wang
> 在 2020年6月5日,03:19,Valerie Peng 写道: > >> 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 fai

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

2020-06-04 Thread 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 wr

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

2020-06-04 Thread Weijun Wang
HmacCore.java: 78 md = null; 79 String noCloneProv = md.getProvider().getName(); NPE on line 79. Should reverse. > On Jun 4, 2020, at 8:09 AM, Valerie Peng wrote: > > Hi, > > Anyone can help review this fix? I changed com.sun.crypto.provider.HmacCore > cl

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

2020-06-03 Thread Valerie Peng
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. Noticed a bug in sun.security.provider.DigestBase class which misses to reset