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

[Weijun Wang]

Good.

Thanks
Max

On 5/5/2015 11:17 PM, Vincent Ryan wrote:
> FYI updated webrev at:
> http://cr.openjdk.java.net/~vinnie/8079129/webrev.01/
>
>
>> On 5 May 2015, at 15:53, Vincent Ryan <vincent.x.ryan at oracle.com
>> <mailto: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
>>> <mailto: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>
>>>>> <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>
>>>>>>> <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>
>>>>>>>>> <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/
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
>