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

[Valerie Peng]

The updated webrev looks good. I will proceed with pre-integration 
testing and push the changes on your behalf once the testing is done.

In the mean time, if you have any particular wording for the commit 
message, please send it my way.

Thanks,
Valerie
On 4/3/2020 5:40 AM, Alexey Bakhtin wrote:
> Hello Valeri,
>
> Thank you for review. Please, have a look at updated patch at : 
> https://cr.openjdk.java.net/~abakhtin/8241960/webrev.v1/
>
> I’ve fixed your findings and renamed test class to ThreadSafetyTest. 
> Jtreg already has ThreadSafeTest class for java.text.Normalizer 
> functionality.
>
> Thank you
> Alexey
>
>> On 3 Apr 2020, at 02:51, Valerie Peng <valerie.peng at oracle.com 
>> <mailto:valerie.peng at oracle.com>> wrote:
>>
>> 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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20200403/a714447d/attachment.htm>