I overlooked that potential race condition when creating the SunJCE singleton.
Both proposed solutions risk the construction of superfluous SunJCE objects. Wouldn't it be better to use the Enum idiom to ensure that multiple SunJCE objects are not constructed? On 29 Mar 2013, at 01:00, 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. > >> >> 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 >> >> >
