Re: 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/


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/


sun.security.util.DerValue.equals(DerValue) method does not check that
null is passed. As a result, NullPointerException can occur.

Artem

Re: 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/ 



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/


sun.security.util.DerValue.equals(DerValue) method does not check that
null is passed. As a result, NullPointerException can occur.

Artem

Re: 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/


sun.security.util.DerValue.equals(DerValue) method does not check that
null is passed. As a result, NullPointerException can occur.

Artem

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

[Vincent Ryan]

All looks good now. Thanks.


On 20/12/2013 14:05, Artem Smotrakov wrote:

Hello Vincent,

Thanks for your feedback.

I heve updated the webrev with the following:
  - the test moved to the existing
test/java/security/cert/X509Certificate/ directory
  - copyright in the test
  - copyright year at the start of DerValue.java

There was no 'othervm' tag in the test. Did I miss something?

Please take a look:
http://cr.openjdk.java.net/~asmotrak/8028431/webrev.01/


Artem

On 12/20/2013 05:19 PM, 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/


sun.security.util.DerValue.equals(DerValue) method does not check that
null is passed. As a result, NullPointerException can occur.

Artem

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

[Artem Smotrakov]

Hello Vincent,

Thanks for your feedback.

I heve updated the webrev with the following:
 - the test moved to the existing 
test/java/security/cert/X509Certificate/ directory

 - copyright in the test
 - copyright year at the start of DerValue.java

There was no 'othervm' tag in the test. Did I miss something?

Please take a look: 
http://cr.openjdk.java.net/~asmotrak/8028431/webrev.01/ 



Artem

On 12/20/2013 05:19 PM, 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/


sun.security.util.DerValue.equals(DerValue) method does not check that
null is passed. As a result, NullPointerException can occur.

Artem

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

[Vincent Ryan]

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/


sun.security.util.DerValue.equals(DerValue) method does not check that
null is passed. As a result, NullPointerException can occur.

Artem