Hi Jamil,
Thanks for the review. Good catch on the formatting typo. I have fixed it.
Removed some of the casting that you mentioned also.
As for only free'ing context upon non-zero return value, it's more for
allowing the java objects to be used again without re-initialization.
When an error/exception occur, we can throw away everything. However,
when operations finish successfully, we re-use the context for
subsequent operations.
The context will later be freed through explicit nativeFree(long) call
when the object is being GC'ed.
Valerie
On 5/12/2016 11:39 AM, Jamil Nimeh wrote:
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
