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

[Matthew Hall]

Shouldn't cache entries be stored using a weak reference as well?
-- 
Sent from my mobile device.

Brad Wetmore <bradford.wetmore at oracle.com> wrote:

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