<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <p>Webrev updated:
      <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~valeriep/8080462/webrev.02/">http://cr.openjdk.java.net/~valeriep/8080462/webrev.02/</a></p>
    <p>Mach5 run looks clean.</p>
    Thanks,<br>
    Valerie<br>
    <div class="moz-cite-prefix">On 6/5/2019 7:42 PM, Valerie Peng
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:24bd05f8-809b-df80-1551-74d732c2e4b4@oracle.com">
      <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
      <p>Hi Jamil,</p>
      <p>Thanks much for reviewing this~<br>
      </p>
      <div class="moz-cite-prefix">On 6/5/2019 9:21 AM, Jamil Nimeh
        wrote:<br>
      </div>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <meta http-equiv="Content-Type" content="text/html;
          charset=UTF-8">
        Hi Valerie, on the whole it looks really good.  I do have some
        comments below:<br>
        <br>
        <ul>
          <li>SunPKCS11.java</li>
          <ul>
            <li>728-738: I think you could add <span class="new">2.16.840.1.101.3.4.3.3
                and .4 for dsa-with-sha384 and dsa-with-sha512,
                respectively.</span></li>
          </ul>
        </ul>
      </blockquote>
      <p>Hmm, I didn't find the oids defined for DSA signature with
        SHA384 and SHA512 digests off oid registry search. Where did you
        find the oids, just curious?</p>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <ul>
            <li><span class="new">790-792: Are you sure that's the right
                OID?  OID lookup shows it as PKCS10 and has </span><span
                class="new">1.2.840.113549.1.1.10 as rsassa-pss.</span></li>
          </ul>
        </ul>
      </blockquote>
      <p>Good catch, I missed one component ".1". In hindsight, it'd be
        nice to have a sun.security,util.OidMapping utility class which
        handles the oid and string name aliasing. It's a pain and error
        prone to repeat these inside the providers. Or, maybe JCA can
        handle this instead of each provider.<br>
      </p>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <ul>
          </ul>
          <li><span class="new">CK_MECHANISM.java</span></li>
          <ul>
            <li><span class="new">171: Can a CK_MECHANISM object be
                reused after the action that calls freeHandle() is
                completed?  If so, would it be a good idea to return the
                value of this.pHandle back to 0 (assuming it is nonzero,
                of course)?</span></li>
          </ul>
        </ul>
      </blockquote>
      The CK_MECHANISM object itself may be reused. However, the
      parameters it contained may change depending on whether the
      engineSetParameter(...) is called again with different value. So,
      the current model is to allocate/set the pHandle for every init
      call and free it after sign/verify call. When it is freed, it must
      be reset to 0. Otherwise it may lead to runtime crash later. <br>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <ul>
          </ul>
          <li><span class="new">CK_RSA_PKCS_PSS_PARAMS.java</span></li>
          <ul>
            <li><span class="new">55-61: Seems like you could replace
                all of removeDash() with just String's replaceFirst("-",
                "") method.<br>
              </span></li>
            <li><span class="new">74: I see in a lot of equals methods
                an identity check as well, like "if (this == o) { return
                true; }" maybe add that in before you check the contents
                of "o"?</span></li>
          </ul>
        </ul>
      </blockquote>
      Sure, changed.<br>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <ul>
          </ul>
          <li><span class="new">Functions.java</span></li>
          <ul>
            <li><span class="new">412: Typo: Vender -> Vendor</span></li>
          </ul>
        </ul>
      </blockquote>
      Fixed.<br>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <ul>
          </ul>
          <li><span class="new">PKCS11.java</span></li>
          <ul>
            <li><span class="new">747, 825: Looks like there's a bit of
                header comment rot.  But I'm guessing that could be said
                of other methods in this file that you have not
                modified.  Think it's worth updating the comments?<br>
              </span></li>
          </ul>
        </ul>
      </blockquote>
      Ok, I updated the comments for whose are modified by this change.<br>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <li><span class="new">PKCS11Constants.java</span></li>
          <ul>
            <li><span class="new">362-363: [Nit] Looks like you've been
                adding deprecated markings for other attributes. 
                According to the header file I'm looking at
                CKA_SECONDARY_AUTH and CKA_AUTH_PIN_FLAGS are
                deprecated.</span></li>
          </ul>
        </ul>
      </blockquote>
      Added.<br>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <ul>
          </ul>
          <li><span class="new">p11convert.c</span></li>
          <ul>
            <li><span class="new">594-613, 1562-1574, 1691, et al.: For
                the cases where you are freeing certain fields within
                the ckParamPtr before returning, what happens to the
                CK_TLS_PRF_PARAMS structure once it has been returned to
                the caller with those fields freed?  Is there any chance
                that the struct is reused?  If so, it might be a good
                idea to NULL those freed pointers out.  If the struct is
                done away with after this function exits then it's fine
                as-is.  It looks like these branches happen on cases
                where an exception is ultimately thrown, but I figured
                I'd ask to be sure.<br>
              </span></li>
          </ul>
        </ul>
      </blockquote>
      When there is a pending exception, we should free natively
      allocated memory inside the same method and then return. The
      structure won't be used.<br>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <ul>
            <li><span class="new"> </span><br>
            </li>
            <li><span class="new">797: You don't need a return here, do
                you?</span></li>
          </ul>
        </ul>
      </blockquote>
      Removed.<br>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <ul>
          </ul>
          <li><span class="new">p11crypt.c</span></li>
          <ul>
            <li><span class="new">146-7: Still need these lines?</span></li>
          </ul>
        </ul>
      </blockquote>
      <p>I removed all the commented out debugging printf calls.
        Hopefully we won't need these again. ;)</p>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <ul>
          </ul>
          <li><span class="new">p11digest.c</span></li>
          <ul>
            <li><span class="new">102: Style nit, can we get a newline
                in here and break up the parameter list as you've done
                in other .c files?</span></li>
          </ul>
        </ul>
      </blockquote>
      Fixed.<br>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <ul>
          </ul>
          <li><span class="new">p11sign.c</span></li>
          <ul>
            <li><span class="new">95: I notice in certain places
                long/jlong values are referenced in the format string as
                %X and sometimes as %lX.  Should we standardize on the
                latter?  Maybe no big deal if you aren't seeing compiler
                warnings.<br>
              </span></li>
          </ul>
        </ul>
      </blockquote>
      Done.<br>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <ul>
            <li><span class="new">136: You might want to make that %u
                (or maybe %lu) so the data length prints as an unsigned
                value.  It's unlikely to see an overflow here, but who
                knows?<br>
              </span></li>
          </ul>
        </ul>
      </blockquote>
      Done.<br>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <ul>
            <li><span class="new">530: Just curious: why do you need a
                return here?  Isn't this a void function?  I don't see
                it in some of the other void functions here.<br>
              </span></li>
          </ul>
        </ul>
      </blockquote>
      Removed.<br>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <li><span class="new">GCMParameters.java</span></li>
          <ul>
            <li><span class="new">49: Is the @since 1.8 correct here? 
                Not sure you need @since for a sun.* family class, but
                it's also not JDK 8.</span></li>
          </ul>
        </ul>
      </blockquote>
      <p>This file is mostly lifted from the one inside SunJCE provider.
        I have changed the @since as well as the copyright years. Was
        debating whether to remove the one in SunJCE provider, but ended
        up just copy it over since this RFE is for PKCS#11 provider and
        want to keep the scope of changes on PKCS11 only.</p>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <ul>
          </ul>
          <li><span class="new">P11PSSSignature.java</span></li>
          <ul>
            <li><span class="new">isDigestEqual(): It seems like you
                could simplify this a bit by "flattening" both the
                stdAlg and givenAlg, removing the first instance of the
                "-" and then do a case-ignore comparison.  Something
                like flatStdAlg = stdAlg.replaceFirst("-", "") and the
                same with flatGivenAlg.  Then just "return
                flatStdAlg.equalsIgnoreCase(flatGivenAlg);"  Maybe I'm
                missing an edge case here, but it seems like it could
                work for all the digest strings you reference in the
                static initializer above.</span></li>
          </ul>
        </ul>
      </blockquote>
      <p>Well, this isDigestEqual() method is mostly for comparing the
        edge case of "SHA-1"/"SHA"/"SHA1". For all other digest
        algorithms, what you suggested would work.</p>
      <p>Will re-test everything and update webrev once the testing
        passes.</p>
      Thanks,<br>
      Valerie<br>
      <blockquote type="cite"
        cite="mid:e07b97f4-34c2-2356-4c77-a809dc0e9ff9@oracle.com">
        <ul>
          <ul>
          </ul>
        </ul>
        <p>--Jamil<br>
        </p>
        <br>
        <div class="moz-cite-prefix">On 4/12/2019 5:05 PM, Valerie Peng
          wrote:<br>
        </div>
        <blockquote type="cite"
          cite="mid:1de69fb8-6679-ca1e-3a1e-ef6f37f52ea6@oracle.com"> <br>
          Anyone has time to review this? Besides the header files
          update, I added support for AES/GCM/NoPadding support. Ran
          into some strange NSS error with RSASSA-PSS signature
          mechanism, so I have not included the PSS signature impl here.
          <br>
          <br>
          RFE: <a class="moz-txt-link-freetext"
            href="https://bugs.openjdk.java.net/browse/JDK-8080462"
            moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8080462</a>
          <br>
          <br>
          Webrev: <a class="moz-txt-link-freetext"
            href="http://cr.openjdk.java.net/~valeriep/8080462/webrev.00/"
            moz-do-not-send="true">http://cr.openjdk.java.net/~valeriep/8080462/webrev.00/</a>
          <br>
          <br>
          CSR: <a class="moz-txt-link-freetext"
            href="https://bugs.openjdk.java.net/browse/JDK-8221442"
            moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8221442</a>
          <br>
          <br>
          Thanks, <br>
          Valerie <br>
          <br>
          <br>
        </blockquote>
        <br>
      </blockquote>
    </blockquote>
  </body>
</html>