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

[Anthony Scarpino]

Update webrev after a talk about getInstance(), along with comment 
embedded below.

http://cr.openjdk.java.net/~mullan/webrevs/ascarpin/7171982/webrev.00/


On 03/28/2013 06:00 PM, Anthony Scarpino wrote:
> 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.

I looked into this further and every try-catch changes the initial 
exception to a runtime exception.  If I changed one, I should probably 
change them all and I'm not comfortable making a change like that with 
this bug fix.  If you feel this is a problem, maybe we can file a 
seperate bug to address this.

Tony

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