On Sat, 31 Jan 2026 00:02:22 GMT, Valerie Peng <[email protected]> wrote:
>> Underlying issue: if provider returns `PKCS11Exception:
>> CKR_ENCRYPTED_DATA_INVALID` instead of BadPaddingException -
>> `java.security.ProviderException: doFinal()` is thrown
>
> test/jdk/sun/security/pkcs11/Cipher/TestPKCS5PaddingError.java line 73:
>
>> 71: // Checking for SunJCE first
>> 72: System.out.println("Checking SunJCE provider");
>> 73: doTest(Security.getProvider("SunJCE"));
>
> I am not too sure if this is really needed as this is the test for PKCS11
> provider. If you really want to keep this, how about adding a static String
> variable whose value is `System.getProperty("test.provider.name", "SunJCE")`?
> This way it's consistent with line 90 inside the `doTest()` method.
I believe it should, as this helped me to debug this issue a lot. Would be very
helpful if this could give us both cases for comparison.
Will add a static var in the next commit
> test/jdk/sun/security/pkcs11/Cipher/TestPKCS5PaddingError.java line 107:
>
>> 105: c2.doFinal(cipherText, 0, cipherText.length
>> - 2);
>> 106: } catch (IllegalBlockSizeException ibe) {
>> 107: // expected
>
> Can we also add a `System.out.println("Expected IBSE thrown");` for
> information sake? Same goes for the other expected exception block on line
> 119.
Actually, what do you think about throwing an exception here, something like
throw new RuntimeException("Expected IBSE thrown");
it shouldn't happen, so failing the test should be valid IMO
> test/jdk/sun/security/pkcs11/Cipher/TestPKCS5PaddingError.java line 127:
>
>> 125: } catch (NoSuchAlgorithmException nsae) {
>> 126: System.out.println("Skipping unsupported algorithm:
>> " +
>> 127: nsae);
>
> nit: why so much indent?
IDE changed automatically, will revert
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29503#discussion_r2758509651
PR Review Comment: https://git.openjdk.org/jdk/pull/29503#discussion_r2758500752
PR Review Comment: https://git.openjdk.org/jdk/pull/29503#discussion_r2758493835
