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

[Brad Wetmore]

Tony,

I knew there was another potential issue but couldn't recall it at the time.

In JCE verification code, we check to see if the provider in the jar 
file has been signed by an appropriate key.  To optimize later provider 
checks, we add the signed provider to a "verified" provider cache.

See:

     jdk/src/share/classes/javax/crypto/JceSecurity.verificationResults

So if you're potentially creating multiple providers, you're 
inadvertently leaking JCE instances.

We ran into this problem 5 years ago:

     http://hg.openjdk.java.net/jdk8/tl/jdk/log?rev=6578538

I might suggest changing to the Holder pattern (Effective Java 2nd 
Edition: Item 71)

Thanks,

Brad



On 3/28/2013 2:45 PM, Brad Wetmore wrote:
> (Whoops, was working on two reviews with two related comments, and
> reversed the emails).
>
> Just realized, there are no regression tests here.
>
> Simplest is to probably do as much setup as you can, then
> java.security.Security.removeProvider("SunJCE"), then issue the calls
> that call into these changes.  They should all pass in the new version,
> and fail in the old.
>
> Brad
>
>
>
> On 3/28/2013 2: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.
>>
>> 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.
>>
>> 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;
>>          }
>>      }
>>
>> 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
>>
>>