Hi Alexey,
In general looks pretty good, just some comments on the regression test:
- line 28: The duration value 10 may be lowered to shorten the execution
time. On a Linux machine, with threadFactor=5, each digest algo takes
about (2xduration+2) sec and overall takes ~283sec. Cutting the duration
value drastically to 2, I still can reproduce the bug. Maybe 4 would be
a better default value considering the execution time.
- line 43: add "SHA-512/224", "SHA-512/256" to the ALGORITHM_ARRAY?
- line 50: instead of hardcoding 5 here, why not just use threadsFactor
(assigned with default value 5 on line 48)?
- line 52-55: I think you meant to use 5 (instead of the value 180) as
the default min duration on line 52. Then, use duration variable instead
of the hardcoded 5 on line54. For testing purpose, why not just use the
supplied value? You already have default values if none are supplied.
- Consider iterating through existing providers and remove
isSHA3supported() method. This way all providers which supports the
digest algorithm are tested and no provider-specific knowledge is
needed. For example:
Provider[] provs = Security.getProviders();
for (Provider p : provs) {
System.out.println("Testing provider " + p.getName() + "...");
for (String alg: ALGORITHM_ARRAY) {
try {
MessageDigest md = MessageDigest.getInstance(alg, p);
testThreadSafe(md, input, nTasks, duration, false);
....
- line 76: missing space char between } and catch.
Thanks,
Valerie
On 3/31/2020 1:01 PM, Valerie Peng wrote:
Hi Alexey,
Good catch, thanks for the report, I will review it.
On a first look, it seems that this is more about the clone() method
of the SHA-3 impl missed copying/cloning an internal field.
Given that this is about SUN provider, I've modified the synopsis
accordingly and move the priority up to P3.
It may not take multi-thread to reproduce this? If so, we can simplify
the regression test.
Regards,
Valerie
On 3/31/2020 11:27 AM, Alexey Bakhtin wrote:
Hi All,
Please review fix for SHA3 message digests thread safety.
Issue reproduced on the JDK11, JDK13 and JDK14
JTREG test is provided in the patch
JBS: https://bugs.openjdk.java.net/browse/JDK-8241960
Webrev: https://cr.openjdk.java.net/~abakhtin/8241960/webrev.v0/
Regards
Alexey
