code review request: 7171982 Cipher getParameters() throws RuntimeException: Cannot find SunJCE provider

[Anthony Scarpino]

On 03/28/2013 02:34 PM, Brad Wetmore wrote:
> (Vinnie, what do you think about the SunJCE item below?)
>
> On 3/22/2013 11:57 AM, Anthony Scarpino wrote:
>> Hi all,
>>
>> I need a code review for below webrev.  The changes are to have SunJCE
>> call itself, using it's current instance, for checking such things as
>> parameters, instead of searching through the provider list or creating a
>> one time instance.
>>
>> http://cr.openjdk.java.net/~mullan/webrevs/ascarpin/webrev.00/
>
> PBES1Core.java
> ==============
> 173:  indention problem.  Should be at the same level as (algo...)
>
> PBES2Core.java:173
> PKCS12PBECipherCore.java:147
> SealedObjectForKeyProtector:50/57
> ========================
> Indention problem. Normally 4 spaces unless you're trying to line it up
> with something.

Looks like I need to change netbeans code formatting as I was letting it 
be my guide for those.

>
> SealedObjectForKeyProtector.java
> ================================
> 54/57:  In general, you should initCause() everywhere you possibly can.
>   This will help people (us) debug the real underlying root cause,
> instead of just the top-level error message.

Sounds reasonable.

>
> SunJCE.java
> ===========
> 781:  Your code could race during initialization and potentially have
> many SunJCE instances active at once.
>
> Either make instance a volatile (will reduce some of the race
> opportunity), or instead, add locking around assignment/use.  You may
> still be creating multiple SunJCEs, but only one instance will ever be
> obtained from getInstance:
>
>      synchronized (SunJCE.class) {
>          if (instance == null) {
>              instance = this;
>          }
>      }
>
> and
>
>      static SunJCE getInstance() {
>          if (instance == null) {
>              new SunJCE();
>          }
>          synchronized (SunJCE.class) {
>              return instance;
>          }
>      }

I think what you have there creates the situation where if two 
getInstance()'s were called with instance = null, the second thread to 
make it through the synchronized call creates a SunJCE object that never 
gets used and returns the first threads object.

Maybe it makes sense to make 'instance' volatile, then:

     SunJCE() { ...
         if (instance == null) {
             instance = this;
         }
     }

and

     static SunJCE getInstance() {
         if (instance == null) {
             return new SunJCE();
         }
         return instance;
     }

We don't stop multiple SunJCE objects, not that stopping them was ever 
the intention of this change, but we reduce their likelihood during a 
crunch and if they are created, at least they are used before being 
discarded.

Tony

>
> Also, when you get ready to push, be sure to address also the closed
> side: that is, please remember to build/integrate the signed
> sunjce_provider.jar file in the closed repo.
>
> HTH,
>
> Brad
>
>