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 <[email protected]>
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
<[email protected]> 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 <[email protected]>
写道：
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
