RFR JDK-8000415: Add support for SHA-3

Valerie Peng valerie.peng at oracle.com
Thu May 12 21:25:25 UTC 2016


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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20160512/b6180b71/attachment.htm>


More information about the security-dev mailing list