On Wed, 22 Oct 2025 14:06:12 GMT, Mikhail Yankelevich <[email protected]> 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
