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

[Sean Mullan]

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