On 5/12/2016 10:38 AM, Anthony Scarpino wrote:
On 05/04/2016 07:08 PM, Valerie Peng wrote:
Hi,
Can someone help reviewing the changes for SHA-3?
The result has been validated against the NIST test vectors (for
BYTE-ONLY impls, i.g. input which are multiples of bytes).
The feature complete date is coming up in a week or two. So, if this can
be reviewed in a week or so, that'd be great.
The changes for SUN providers are quite straight-forward, e.g. SHA-3
digest impls based on FIPS PUB 202.
As for OracleUcrypto provider, Solaris SHA-3 support is through new
libucrypto digest APIs (added in Solaris 12) instead of the libmd.
When running on Solaris 12, the new libucrypto APIs will be used.
Otherwise, libmd will be used.
Changes for OracleUcrypto providers:
- add JNI code for the new libucrypto digest APIs
- code refactoring, e.g. move the libmd-related code to classes with MD
suffix
- run-time mechanism number assignment (used to be hardcoded values)
- better error reporting
RFE: https://bugs.openjdk.java.net/browse/JDK-8000415
Webrev: http://cr.openjdk.java.net/~valeriep/8000415/webrev.00/
Thanks,
Valerie
Just a few minor things.
UcryptoProvider.java:161
Did you mean to only comment it out and not delete it?
nativeFunc.c:
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.
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.
Tony
Hi Valerie, just a few additional nits - not a big deal either way:
* nativeCryptoMD.c:
o 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.
o 123, 125: I think these may be unnecessary casts, too.
o 133,135,137: pretty sure free doesn't need casts. It likes all
pointers regardless of type.
* nativeCrypto.c:
o 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.
o 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.
o 362: Since context is a pointer, better to use NULL rather than
zero.
--Jamil
