JDK 8 Review Request for 6854712 (JEP 124), 6637288 and 7126011

[Sean Mullan]

On 5/22/12 5:20 PM, David Schlosnagle wrote:
> 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.

Great. Thanks for taking the time to look them over.

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

I did not -- instead I followed our current practice when adding new methods to
the *Spi abstract classes that don't have a reasonable default implementation.
See, for example CertificateFactorySpi.engineGenerateCertPath.

Let me think about this some more. It might be worth considering something like
that but I probably won't make any changes for this current revision.

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

Good point. Will fix.

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

They are not intended to be thread safe. These are internal classes of the PKIX
CertPathBuilder and CertPathValidator implementation. The javadocs document the
thread-safeness in the class summary, ex for CertPathValidator:

"The static methods of this class are guaranteed to be thread-safe. Multiple
threads may concurrently invoke the static methods defined in this class with no
ill effects.

However, this is not true for the non-static methods defined by this class.
Unless otherwise documented by a specific provider, threads that need to access
a single CertPathValidator instance concurrently should synchronize amongst
themselves and provide the necessary locking. Multiple threads each manipulating
a different CertPathValidator instance need not synchronize."

These *Checker classes are package-private, so I think the code is ok to leave
as-is. If we were to make them public and make them available to applications,
then I agree we should fix the escaped mutable state issue you pointed out.

> 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});

Good point, will fix.

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

Good point, will fix.

> 
> 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, will fix.

--Sean