code review request: 7171982 Cipher getParameters() throws RuntimeException: Cannot find SunJCE provider
Anthony Scarpino
anthony.scarpino at oracle.com
Fri Mar 29 01:00:03 UTC 2013
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
>
>
More information about the security-dev
mailing list