<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<br>
<div class="moz-cite-prefix">On 5/12/2016 10:38 AM, Anthony Scarpino
wrote:<br>
</div>
<blockquote cite="mid:5734BF7A.5070706@oracle.com" type="cite">On
05/04/2016 07:08 PM, Valerie Peng wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
Can someone help reviewing the changes for SHA-3?
<br>
<br>
The result has been validated against the NIST test vectors (for
<br>
BYTE-ONLY impls, i.g. input which are multiples of bytes).
<br>
The feature complete date is coming up in a week or two. So, if
this can
<br>
be reviewed in a week or so, that'd be great.
<br>
<br>
The changes for SUN providers are quite straight-forward, e.g.
SHA-3
<br>
digest impls based on FIPS PUB 202.
<br>
As for OracleUcrypto provider, Solaris SHA-3 support is through
new
<br>
libucrypto digest APIs (added in Solaris 12) instead of the
libmd.
<br>
When running on Solaris 12, the new libucrypto APIs will be
used.
<br>
Otherwise, libmd will be used.
<br>
Changes for OracleUcrypto providers:
<br>
- add JNI code for the new libucrypto digest APIs
<br>
- code refactoring, e.g. move the libmd-related code to classes
with MD
<br>
suffix
<br>
- run-time mechanism number assignment (used to be hardcoded
values)
<br>
- better error reporting
<br>
<br>
RFE: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8000415">https://bugs.openjdk.java.net/browse/JDK-8000415</a>
<br>
Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~valeriep/8000415/webrev.00/">http://cr.openjdk.java.net/~valeriep/8000415/webrev.00/</a>
<br>
<br>
Thanks,
<br>
Valerie
<br>
</blockquote>
<br>
Just a few minor things.
<br>
<br>
UcryptoProvider.java:161
<br>
Did you mean to only comment it out and not delete it?
<br>
<br>
nativeFunc.c:
<br>
I noticed it is checking the library functions via dlsym. You
could have a conditional check on the version of the library via
ucrypto_version, then do the dlsym calls if it is supported.
<br>
<br>
<br>
I think there is a performance enhancement that I would not hold
up for this changeset. Changing the byte[]'s to long[] might give
a good performance gain. A similar change was done to GHASH.java
which resulted in a 10x performance increase. Given the internal
methods appear to use long[][], and lanes are in longs it would
probably help. But it most likely require a change to add
DigestBase.implCompress(long[], int) and DigestBase.and
implDigest(long[], int). Probably a low priority RFE.
<br>
<br>
Tony
<br>
<br>
</blockquote>
Hi Valerie, just a few additional nits - not a big deal either way:<br>
<ul>
<li>nativeCryptoMD.c:</li>
<ul>
<li>47, 53, 59, 65, etc.: You don't need to cast the pointer
from malloc since you're assigning it to a void *. The
casting only needs to happen when you go to use the pointer
with a function that expects a concrete type. I tried it with
gcc and cc with full warnings just to be sure. But it's no
problem to leave it in there as-is.</li>
<li>123, 125: I think these may be unnecessary casts, too.</li>
<li>133,135,137: pretty sure free doesn't need casts. It likes
all pointers regardless of type.</li>
</ul>
<li>nativeCrypto.c:</li>
<ul>
<li>40,42: I'm surprised you're not getting warnings on these,
as "0x0x" isn't a valid format specifier so you have a
mismatch in terms of arguments vs format specifiers. Do you
mean 0x%0x? The latter works ok.</li>
<li>357: Just a question, does ucryptoDigestFinal automatically
free the context as part of the finalize? Otherwise you're
only freeing the context on a non-zero return value from that
function. I'm assuming that the context should be freed
regardless of the outcome given the header comment.</li>
<li>362: Since context is a pointer, better to use NULL rather
than zero.</li>
</ul>
</ul>
<p>--Jamil<br>
</p>
<br>
<br>
<br>
</body>
</html>