JDK 8 Review Request for 6854712 (JEP 124), 6637288 and 7126011
David Schlosnagle
schlosna at gmail.com
Tue May 22 21:20:54 UTC 2012
On Tue, May 22, 2012 at 12:09 PM, Sean Mullan <sean.mullan at oracle.com> wrote:
> Can you please review my changes for 6854712 (JEP 124). It also includes
> fixes for a couple of other CRs: 6637288 and 7126011. Other reviewers are
> welcome too.
>
> The webrev:
> http://cr.openjdk.java.net/~mullan/webrevs/6854712_6637288_7126011/webrev.00/
Sean,
I had a few random questions and minor comments on this change set.
Have you considered any changes to the APIs to add a public boolean
isRevocationCheckingSupported() like method that indicates whether use of
getRevocationChecker() is supported so that a caller doesn't need to catch the
UnsupportedOperationException?
src/share/classes/sun/security/provider/certpath/AdjacencyList.java
src/share/classes/sun/security/provider/certpath/ForwardState.java
src/share/classes/sun/security/provider/certpath/ReverseState.java
src/share/classes/sun/security/provider/certpath/Vertex.java
src/share/classes/sun/security/provider/certpath/X509CertificatePair.java
The toString() methods could benefit from converting string concatenation
to StringBuilder.append to avoid unnecessary additional StringBuilders
(-XX:+OptimizeStringConcat might take care of these as well).
src/share/classes/sun/security/provider/certpath/ConstraintsChecker.java
src/share/classes/sun/security/provider/certpath/KeyChecker.java
src/share/classes/sun/security/provider/certpath/PolicyChecker.java
Are these classes intended to be thread safe? If so, then
getSupportedExtensions() has a race on supportedExts instance field and
should probably be volatile with double checked lock or thread safe holder
idiom. It also seems like the mutable supportedExts could potentially
escape under the race, so it might be worth building the supported
extensions in a local set and then assigning to this.supportedExts. If they
are not expected to be thread safe, you're probably fine although you might
still want to avoid escaping mutable state and add JavaDoc similar to what
you put in the PKIXRevocationChecker class docs.
src/share/classes/sun/security/provider/certpath/OCSPRequest.java
Is it worth defining/using a constant for the OCSP OID
"1.3.6.1.5.5.7.48.1.2" and using it on line 120 of encodeBytes()?
OCSPResponse has a
private constant ObjectIdentifier OCSP_NONCE_EXTENSION_OID that could
possibly be reused within the package:
136 private static final ObjectIdentifier OCSP_NONCE_EXTENSION_OID =
137 ObjectIdentifier.newInternal(new int[] { 1, 3, 6, 1, 5,
5, 7, 48, 1, 2});
src/share/classes/sun/security/provider/certpath/PolicyNodeImpl.java
Stylistic nit - toString could use for-each:
183 for (PolicyNodeImpl node : getChildren()) {
184 buffer.append(node);
185 }
src/share/classes/sun/security/provider/certpath/SunCertPathBuilder.java
In depthFirstSearchForward, the finalCert logic could be optimized to avoid
traversing the entire linked list, possibly twice:
559 if (cpList.isEmpty()) {
560 finalCert = builder.trustAnchor.getTrustedCert();
561 } else {
562 finalCert = cpList.getLast();
563 }
Thanks,
Dave
More information about the security-dev
mailing list