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
