<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Sean,<br>
<br>
Thanks for your feedback.<br>
<br>
I have updated the webrev with your suggestions.<br>
<br>
The test used a real certificate issued by a CA. I created bad
self-signed certificate.<br>
<br>
Please take a look:
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<a
href="http://cr.openjdk.java.net/%7Easmotrak/8028431/webrev.02/">http://cr.openjdk.java.net/~asmotrak/8028431/webrev.02/</a><br>
<br>
Artem<br>
<br>
On 12/20/2013 06:34 PM, Sean Mullan wrote:<br>
</div>
<blockquote cite="mid:52B45586.5030505@oracle.com" type="cite">A
couple of other comments:
<br>
<br>
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.
<br>
<br>
2. Move the "==" check and make it the first thing you check
<br>
<br>
3. Nit: don't include space between "!" and "("
<br>
<br>
@Override
<br>
public boolean equals(Object o) {
<br>
if (this == o) {
<br>
return true;
<br>
}
<br>
if (!(o instanceof DerValue)) {
<br>
return false;
<br>
}
<br>
DerValue other = (DerValue) o;
<br>
<br>
4. In the test, close the FileInputStream after you are done with
it (use try-with-resources)
<br>
<br>
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.
<br>
<br>
Thanks,
<br>
Sean
<br>
<br>
<br>
On 12/20/2013 08:19 AM, Vincent Ryan wrote:
<br>
<blockquote type="cite">Hello Artem,
<br>
<br>
You fix looks good. You just need to fill in the missing portion
<br>
of the copyright in the test. You could also adjust the
copyright
<br>
year range at the start of DerValue.java.
<br>
<br>
Also I would add the test to the existing
<br>
test/java/security/cert/X509Certificate/ directory rather than
create a
<br>
new one.
<br>
<br>
Finally, I think the test should run fine without the jtreg
<br>
tag for 'othervm'.
<br>
<br>
Thanks.
<br>
<br>
<br>
On 20/12/2013 12:51, Artem Smotrakov wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
please review this fix for 9:
<br>
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8028431">https://bugs.openjdk.java.net/browse/JDK-8028431</a>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~asmotrak/8028431/webrev.00/">http://cr.openjdk.java.net/~asmotrak/8028431/webrev.00/</a>
<br>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Easmotrak/8028431/webrev.00/"><http://cr.openjdk.java.net/%7Easmotrak/8028431/webrev.00/></a>
<br>
<br>
sun.security.util.DerValue.equals(DerValue) method does not
check that
<br>
null is passed. As a result, NullPointerException can occur.
<br>
<br>
Artem
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>