<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
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>
<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>
<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>
<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>
<li><span class="new">Functions.java</span></li>
<ul>
<li><span class="new">412: Typo: Vender -> Vendor</span></li>
</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>
<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>
<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>
<li><span class="new">797: You don't need a return here, do you?</span></li>
</ul>
<li><span class="new">p11crypt.c</span></li>
<ul>
<li><span class="new">146-7: Still need these lines?</span></li>
</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>
<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>
<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>
<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>
<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>
<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>
<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">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/">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">https://bugs.openjdk.java.net/browse/JDK-8221442</a>
<br>
<br>
Thanks,
<br>
Valerie
<br>
<br>
<br>
</blockquote>
<br>
</body>
</html>