On Wed, 3 Apr 2024 18:11:34 GMT, Valerie Peng <valer...@openjdk.org> wrote:

>> src/jdk.crypto.cryptoki/unix/native/libj2pkcs11/p11_md.c line 94:
>> 
>>> 92:     char *systemErrorMessage;
>>> 93:     char *exceptionMessage;
>>> 94:     const char *getFunctionListStr = "C_GetFunctionList";
>> 
>> If this value ever gets used by ReleaseStringUTFChars, the failure will be 
>> spectacular 🍿
>
> We do have checks for jGetFunctionList != NULL before calling 
> ReleaseStringUTFChars() with it. So, this shouldn't be an issue?

this will be an issue if `jGetFunctionList != NULL` and `dlopen` fails, 
resulting in `goto cleanup` before `getFunctionListStr` is reassigned.

>> src/jdk.crypto.cryptoki/unix/native/libj2pkcs11/p11_md.c line 165:
>> 
>>> 163:             if (ckAssertReturnValueOK(env, rv) == CK_ASSERT_OK) {
>>> 164:                 goto setModuleData;
>>> 165:             }
>> 
>> Do we need an `else goto cleanup` here?
>
> Not really, the intention is to continue with the C_GetFunctionList (or the 
> method named by "getFunctionListStr").

if that's the intention, then you shouldn't be using `ckAssertReturnValueOK`, 
which throws an exception if `rv` is anything other than `CK_ASSERT_OK`

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18588#discussion_r1550245099
PR Review Comment: https://git.openjdk.org/jdk/pull/18588#discussion_r1550243300

Reply via email to