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

[Artem Smotrakov]

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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20131224/e0eebd34/attachment.htm>