RFR JDK-8134364: Add defensive copies to get/set methods for OCSPNonceExtension

[Sean Mullan]

Looks good.

--Sean

On 09/01/2015 09:14 PM, Jamil Nimeh wrote:
> Hi Sean, et al.,
>
> I've updated the review to incorporate Sean's comments.  Sean, I was
> able to remove encodeInternal() and make it into a one-liner inside each
> of the ctors, so that shrunk things down a bit more which is nice.  Let
> me know what you think.
>
> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8134364/webrev.02/
>
> Thanks,
> --Jamil
>
> On 09/01/2015 11:29 AM, Sean Mullan wrote:
>> On 08/28/2015 09:25 PM, Jamil Nimeh wrote:
>>> Hello all,
>>>
>>> I've removed the CertAttrSet interface from OCSPNonceExtension and
>>> trimmed away a few unneeded methods.  As a result the class is immutable
>>> now.
>>
>> Looks a lot cleaner. Strictly speaking, the cloning is not necessary
>> since this is an internal class -- unless the array can leak through
>> some code path to untrusted code, but I think in this case it is good
>> practice and probably won't affect performance much as nonces are a
>> rarely used feature (I think). A few comments (some stylistic based on
>> [1]) on OCSPNonceExtension:
>>
>> 48: make class final
>>
>> 49: constants are generally named upper case and with "_" as a word
>> separator, so EXTENSION_NAME is better.
>>
>> 58: move this private method after the constructors (or better yet,
>> embed it inside the ctor since it is only called from there)
>>
>> 84: not necessarily the SUN provider, depends on the configured
>> provider order
>>
>> 174: toString should probably use StringBuilder.append instead of "+".
>>
>> --Sean
>>
>> [1] review ongoing at
>> http://cr.openjdk.java.net/~alundblad/styleguide/feedback.html
>>
>>>
>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8134364
>>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8134364/webrev.01
>>>
>>> Thanks,
>>> --Jamil
>>>
>>> On 08/25/2015 08:11 AM, Jamil Nimeh wrote:
>>>> Hi Sean,
>>>>
>>>> Yes, I was just following Extension convention in terms of
>>>> implementing CertAttrSet.  Within sun.security.x509, X509CertInfo uses
>>>> the set methods from other objects implementing CertAttrSet. But
>>>> OCSPNonceExtension really isn't a certificate attribute so it probably
>>>> doesn't need to bring this interface in.
>>>>
>>>> I'll refactor this and alter the tests to match.
>>>>
>>>> --Jamil
>>>>
>>>> On 08/25/2015 06:40 AM, Sean Mullan wrote:
>>>>> Is it necessary for the class to implement CertAttrSet? I realize all
>>>>> of the internal X.509 extensions implement that, but that was done a
>>>>> long time ago and not something we really want to carry forward if
>>>>> not needed. I think it would be cleaner not to do that as it would
>>>>> more easily allow you to make the class immutable, and would allow
>>>>> you to remove the set method.
>>>>>
>>>>> Thanks,
>>>>> Sean
>>>>>
>>>>> On 08/25/2015 04:14 AM, Xuelei Fan wrote:
>>>>>> OCSPNonceExtension.java
>>>>>> =======================
>>>>>> -      nonceData = (byte[])obj;
>>>>>> +      nonceData = ((byte[])obj).clone();
>>>>>>
>>>>>> Do you want to check null obj?
>>>>>>
>>>>>> -      return nonceData;
>>>>>> +      return (nonceData != null ? nonceData.clone() : null);
>>>>>>
>>>>>> I think you may want to enclose the "!=" operator as:
>>>>>>
>>>>>> +      return (nonceData != null) ? nonceData.clone() : null;
>>>>>>
>>>>>>
>>>>>> Xuelei
>>>>>>
>>>>>> On 8/25/2015 12:55 PM, Jamil Nimeh wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> This is a quick fix to the OCSPNonceExtension class to add a couple
>>>>>>> defensive copies to public get/set methods.
>>>>>>>
>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8134364
>>>>>>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8134364/webrev.00
>>>>>>>
>>>>>>> Thanks,
>>>>>>> --Jamil
>>>>>>
>>>>
>>>
>