On Fri, 31 Jan 2025 17:29:24 GMT, Matthew Donovan <[email protected]> wrote:
>> Earlier code will trigger NPE if the certificate does not contain the
>> extensions or if the requested extensions does not exist. The better
>> approach for hardening **getExtensionValue** here is to to check for NULL
>> explicitly before calling **getExtensionValue()** and avoding try-catch
>> block which ensures the readability and maintainability.
>>
>> After scanning in multiple places where invokng getExtensions on the
>> X509CertInfo reference, the check for NULL is added in the **getKeyUsage()**
>> as well while calling before **getExtensionValue()**
>>
>> The associated tests are written and added in test class
>> **CertificateExtensions**. Which will ensure to validate the
>> **getExtensionValue()** and **getKeyUsage()** methods in **X509CertImpl**
>> class.
>
> test/jdk/java/security/cert/X509Certificate/CertificateExtensions.java line 1:
>
>> 1:
>
> Needs a copyright statement
added the copyright statement
> test/jdk/java/security/cert/X509Certificate/CertificateExtensions.java line
> 29:
>
>> 27: * else it is incorrect
>> 28: */
>> 29: try {
>
> These try/catch blocks around the assert statements seem redundant. The
> assert methods already throw a RuntimeException with a message.
Addressed and consolidated all try-catch blocks
> test/jdk/java/security/cert/X509Certificate/CertificateExtensions.java line
> 40:
>
>> 38: */
>> 39: try {
>> 40: System.out.println("------ cert x509Certimpl ------ " +
>> x509Certimpl);
>
> The output might be a little easier to read if you put the "---- cert
> X509Certimpl ----" on its own line.
was added these during the debug and continued the same in draft PR. I have
removed these statements
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23315#discussion_r1940501046
PR Review Comment: https://git.openjdk.org/jdk/pull/23315#discussion_r1940502406
PR Review Comment: https://git.openjdk.org/jdk/pull/23315#discussion_r1940501952
