[9] request for review 8079129: NullPointerException in PKCS#12 Keystore in PKCS12KeyStore.java

[Vincent Ryan]

FYI updated webrev at:
  http://cr.openjdk.java.net/~vinnie/8079129/webrev.01/ <http://cr.openjdk.java.net/~vinnie/8079129/webrev.01/>


> On 5 May 2015, at 15:53, Vincent Ryan <vincent.x.ryan at oracle.com> wrote:
> 
> I’ll skip the initialization.
> Thanks.
> 
> 
>> On 5 May 2015, at 15:52, Weijun Wang <weijun.wang at oracle.com> wrote:
>> 
>> That's good, but there is no need to assign null in
>> 
>>      Certificate[] certs = null;
>> 
>> Or, maybe you can add "if (certs != null)" around the loop, but you might not like an extra indentation.
>> 
>> --Max
>> 
>> On 5/5/2015 10:44 PM, Vincent Ryan wrote:
>>> OK. How about this?
>>> 
>>> --- a/src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java
>>> +++ b/src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java
>>> @@ -1,5 +1,5 @@
>>> /*
>>> - * Copyright (c) 1999, 2014, Oracle and/or its affiliates. All rights
>>> reserved.
>>> + * Copyright (c) 1999, 2015, Oracle and/or its affiliates. All rights
>>> reserved.
>>>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>  *
>>>  * This code is free software; you can redistribute it and/or modify it
>>> @@ -1642,23 +1642,22 @@
>>>             Entry entry = entries.get(alias);
>>> 
>>> 
>>>             // certificate chain
>>> -            int chainLen = 1;
>>>             Certificate[] certs = null;
>>> 
>>> 
>>>             if (entry instanceof PrivateKeyEntry) {
>>>                 PrivateKeyEntry keyEntry = (PrivateKeyEntry) entry;
>>> -                    if (keyEntry.chain == null) {
>>> -                        chainLen = 0;
>>> -                    } else {
>>> -                        chainLen = keyEntry.chain.length;
>>> -                    }
>>> -                certs = keyEntry.chain;
>>> -
>>> +                if (keyEntry.chain != null) {
>>> +                    certs = keyEntry.chain;
>>> +                } else {
>>> +                    certs = new Certificate[0];
>>> +                }
>>>             } else if (entry instanceof CertEntry) {
>>> -               certs = new Certificate[]{((CertEntry) entry).cert};
>>> +                certs = new Certificate[]{((CertEntry) entry).cert};
>>> +            } else {
>>> +                certs = new Certificate[0];
>>>             }
>>> 
>>> 
>>> -            for (int i = 0; i < chainLen; i++) {
>>> +            for (int i = 0; i < certs.length; i++) {
>>>                 // create SafeBag of Type CertBag
>>>                 DerOutputStream safeBag = new DerOutputStream();
>>>                 safeBag.putOID(CertBag_OID);
>>> 
>>> 
>>> 
>>>> On 5 May 2015, at 15:10, Weijun Wang <weijun.wang at oracle.com
>>>> <mailto:weijun.wang at oracle.com>> wrote:
>>>> 
>>>> Anyway it looks redundant and error-prone to maintain the length of an
>>>> array in a separate variable.
>>>> 
>>>> --Max
>>>> 
>>>> On 5/5/2015 8:32 PM, Vincent Ryan wrote:
>>>>> Replacing the for loop below with a for-each loop on certs would be
>>>>> fine except that certs can be null.
>>>>> I could initialize certs with an empty array on each iteration of the
>>>>> outer loop but it doesn’t seem to gain much overall.
>>>>> 
>>>>> 
>>>>>> On 4 May 2015, at 13:10, Weijun Wang <weijun.wang at oracle.com
>>>>>> <mailto:weijun.wang at oracle.com>> wrote:
>>>>>> 
>>>>>> 1662             for (int i = 0; i < chainLen; i++) {
>>>>>> 
>>>>>> 
>>>>>> On 5/4/2015 6:08 PM, Vincent Ryan wrote:
>>>>>>> Which line?
>>>>>>> 
>>>>>>>> On 2 May 2015, at 02:22, Weijun Wang <weijun.wang at oracle.com
>>>>>>>> <mailto:weijun.wang at oracle.com>> wrote:
>>>>>>>> 
>>>>>>>> Is it safe to just run for-each on certs (if it's not null)?
>>>>>>>> 
>>>>>>>> --Max
>>>>>>>> 
>>>>>>>> On 5/2/2015 6:39 AM, Vincent Ryan wrote:
>>>>>>>>> Please review this fix to correct the PKCS12 encoding when a
>>>>>>>>> secret key is being stored in one keystore entry and a
>>>>>>>>> certificate in another.
>>>>>>>>> 
>>>>>>>>> Thanks.
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8079129
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~vinnie/8079129/webrev.00/
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20150505/d757a9d5/attachment.htm>