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

[Sean Mullan]

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