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

[Weijun Wang]

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/
>>>>>>>
>>>>>
>>>
>