<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>