(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
