Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
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 Signature class especially for callers requesting Signature with algorithm only which uses the delayed provider selection mechanism. Pondered for a moment whether to do this as it would involve filing a CSR. Given the main focus is MessageDigest and none of the JDK provider support cloneable signature impl, I didn't include this in the 8246077 fix. Thanks, Valerie On 6/15/2020 6:18 PM, Weijun Wang wrote: 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 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 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 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 写道： 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 bec
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
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 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 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 > 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 写道： > Can you give an example when these 2 rules have different > results? Is this only true for those implementation that directly > subclass MessageDigest? >>>
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
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 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 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 写道： 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
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
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 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 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 写道： 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
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
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 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 >> 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 写道： > >> 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 >
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
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 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 写道： 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
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
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? It sounds like a good idea to me if the repeatedly checking could be avoid. Xuelei 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 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 写道： 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
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
"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 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 写道： 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
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
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 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 写道： 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
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
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 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 写道： 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
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
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 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 写道： > 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
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
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 写道： 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
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
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 写道： 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
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
> 在 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 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
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
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 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
Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable
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 > 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? 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. 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 >
