RFR 8241960: The SHA3 message digests are not thread safe when cloned

[Valerie Peng]

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
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20200402/5f833465/attachment.htm>