<html>
<head>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
</head>
<body>
<p>Hi Alexey,</p>
<p>In general looks pretty good, just some comments on the
regression test:</p>
<p>- 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. <br>
</p>
<p>- line 43: add "SHA-512/224", "SHA-512/256" to the
ALGORITHM_ARRAY?<br>
</p>
<p>- line 50: instead of hardcoding 5 here, why not just use
threadsFactor (assigned with default value 5 on line 48)?</p>
<p>- 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.</p>
<p>- 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:</p>
<pre> Provider[] provs = Security.getProviders();</pre>
<pre> for (Provider p : provs) {</pre>
<pre> System.out.println("Testing provider " + p.getName() + "...");</pre>
<pre> for (String alg: ALGORITHM_ARRAY) {</pre>
<pre> try {</pre>
<pre> MessageDigest md = MessageDigest.getInstance(alg, p);</pre>
<pre> testThreadSafe(md, input, nTasks, duration, false);</pre>
<pre> .... </pre>
<p>- line 76: missing space char between } and catch.</p>
Thanks,<br>
Valerie<br>
<div class="moz-cite-prefix">On 3/31/2020 1:01 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:784d0590-357d-1da0-ba01-49fed43ad6b3@oracle.com">Hi
Alexey,
<br>
<br>
Good catch, thanks for the report, I will review it.
<br>
<br>
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.
<br>
<br>
Given that this is about SUN provider, I've modified the synopsis
accordingly and move the priority up to P3.
<br>
<br>
It may not take multi-thread to reproduce this? If so, we can
simplify the regression test.
<br>
<br>
Regards,
<br>
Valerie
<br>
On 3/31/2020 11:27 AM, Alexey Bakhtin wrote:
<br>
<blockquote type="cite">Hi All,
<br>
<br>
Please review fix for SHA3 message digests thread safety.
<br>
Issue reproduced on the JDK11, JDK13 and JDK14
<br>
JTREG test is provided in the patch
<br>
<br>
JBS: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8241960">https://bugs.openjdk.java.net/browse/JDK-8241960</a>
<br>
Webrev: <a class="moz-txt-link-freetext" href="https://cr.openjdk.java.net/~abakhtin/8241960/webrev.v0/">https://cr.openjdk.java.net/~abakhtin/8241960/webrev.v0/</a>
<br>
<br>
Regards
<br>
Alexey
<br>
<br>
</blockquote>
</blockquote>
</body>
</html>