Shouldn't cache entries be stored using a weak reference as well? -- Sent from my mobile device.
Brad Wetmore <bradford.wetm...@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 >>> >>>
