Code Review request: 8028431: NullPointerException in DerValue.equals(DerValue)

[Sean Mullan]

Hi Artem,

The updated fix looks good.

--Sean

On 12/24/2013 09:19 AM, Artem Smotrakov wrote:
> Hi Sean,
>
> Thanks for your feedback.
>
> I have updated the webrev with your suggestions.
>
> The test used a real certificate issued by a CA. I created bad
> self-signed certificate.
>
> Please take a look:
> http://cr.openjdk.java.net/~asmotrak/8028431/webrev.02/
> <http://cr.openjdk.java.net/%7Easmotrak/8028431/webrev.02/>
>
> Artem
>
> On 12/20/2013 06:34 PM, Sean Mullan wrote:
>> A couple of other comments:
>>
>> 1. Add an @Override annotation to the equals method. While you are in
>> there, could you also add @Override to the toString and hashCode methods.
>>
>> 2. Move the "==" check and make it the first thing you check
>>
>> 3. Nit: don't include space between "!" and "("
>>
>>     @Override
>>     public boolean equals(Object o) {
>>         if (this == o) {
>>             return true;
>>         }
>>         if (!(o instanceof DerValue)) {
>>             return false;
>>         }
>>         DerValue other = (DerValue) o;
>>
>> 4. In the test, close the FileInputStream after you are done with it
>> (use try-with-resources)
>>
>> 5. Is the certificate used in the test a real certificate issued by a
>> CA or one that you created yourself? If it is a real certificate, we
>> should not include it in openJDK. You will need to move the test to
>> the closed repo, or create your own bad certificate with the symptoms.
>>
>> Thanks,
>> Sean
>>
>>
>> On 12/20/2013 08:19 AM, Vincent Ryan wrote:
>>> Hello Artem,
>>>
>>> You fix looks good. You just need to fill in the missing portion
>>> of the copyright in the test. You could also adjust the copyright
>>> year range at the start of DerValue.java.
>>>
>>> Also I would add the test to the existing
>>> test/java/security/cert/X509Certificate/ directory rather than create a
>>> new one.
>>>
>>> Finally, I think the test should run fine without the jtreg
>>> tag for 'othervm'.
>>>
>>> Thanks.
>>>
>>>
>>> On 20/12/2013 12:51, Artem Smotrakov wrote:
>>>> Hi,
>>>>
>>>> please review this fix for 9:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8028431
>>>> http://cr.openjdk.java.net/~asmotrak/8028431/webrev.00/
>>>> <http://cr.openjdk.java.net/%7Easmotrak/8028431/webrev.00/>
>>>>
>>>> sun.security.util.DerValue.equals(DerValue) method does not check that
>>>> null is passed. As a result, NullPointerException can occur.
>>>>
>>>> Artem
>>
>