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
