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

[Vincent Ryan]

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