RFR: 8369995: Implement extra logging and/or propagate errors in X509KeyManagerImpl and fix StringIndexOutOfBoundsException [v9]

[Bradford Wetmore]

On Wed, 22 Oct 2025 14:06:12 GMT, Mikhail Yankelevich <myankelevich at openjdk.org> wrote:

>> In [JDK-8309667](https://bugs.openjdk.org/browse/JDK-8309667), there were issues with debugging due to no logging or throwing of errors by X509KeyManagerImpl::getEntry. [Line](https://github.com/openjdk/jdk/blob/6a4c2676a6378f573bd58d1bc32b57765d756291/src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java#L243-L245)
>> 
>> Extra logging and error propagating should be implemented for the X509KeyManagerImpl.
>> 
>> Additionally, dot checking logic has been changed, so no cases similar to `.A` will not trigger StringOutOfBounds exceptions. 
>> 
>> Thank you @djelinski for finding the issue and analysis.
>
> Mikhail Yankelevich has updated the pull request incrementally with one additional commit since the last revision:
> 
>   line length changed

Some issues to address, thanks.

src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 256:

> 254:                  NumberFormatException |
> 255:                  NoSuchAlgorithmException |
> 256:                  IndexOutOfBoundsException e) {

I always worry about enumerating all possible checked exceptions, as we might now start throwing new `RuntimeException`s that were previously caught/ignored.  Was there a reason to drive the change to all checked?

More importantly, will this change now cause new behavior due to RTEs?

test/jdk/sun/security/ssl/X509KeyManager/NullCases.java line 103:

> 101:         final X509KeyManager km = generateNullKm();
> 102: 
> 103:         km.getServerAliases(null, null);

I know that this section is not part of this change, but shouldn't we be testing that all these are indeed returning null?  

It's done in spots later on, but I feel it should be done here also to test all of the APIs.

test/jdk/sun/security/ssl/X509KeyManager/NullCases.java line 203:

> 201:                         "Should return null if the alias is invalid <%s>",
> 202:                         alias));
> 203:     }

Based on the description of the bug, I was expecting to see checking of the debug output (`Implement extra logging...`), but appears that this is not the primary focus.  Maybe change the ordering of the bug description.  (`...and also implement extra logging`)

test/jdk/sun/security/ssl/X509KeyManager/X509KeyManagerNegativeTests.java line 70:

> 68: 
> 69:         exceptionThrowingKMF
> 70:                 .init((KeyStore) exceptionThrowingKS, null);

This cast seems unnecessary.

(Test runs without it.)

test/jdk/sun/security/ssl/X509KeyManager/X509KeyManagerNegativeTests.java line 83:

> 81:     @Test
> 82:     public void ksExceptionTest() {
> 83:         // recording logs to the output stream

I don't understand this comment.  There doesn't seem to be any recording to the debug log stream in this test.

test/jdk/sun/security/ssl/X509KeyManager/X509KeyManagerNegativeTests.java line 103:

> 101:         @Override
> 102:         public KeyStore.Entry engineGetEntry(String alias,
> 103:                                              KeyStore.ProtectionParameter param) {

A line <= 80. 

All the other lines are fine, thanks!

-------------

Changes requested by wetmore (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27851#pullrequestreview-3372319617
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2457035908
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2457098558
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2457170495
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2457180801
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2457249393
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2457206166