Skip to content

Commit d1b43a3

Browse files
[Android] Fix X509Store cleanup (#128629)
Fixes Android X509Store PAL cleanup paths for certificate/private-key entries and JNI local references to avoid memory leaks. Follow-up to #128284 ## Changes - Dispose the Android `KeyStore.PrivateKeyEntry` wrapper held by `AndroidCertificatePal`. - Release JNI local references on Android X509Store cleanup paths: - trusted certificate enumeration - default store open failure/success cleanup - remove-certificate early success path - Add JNI exception checks when advancing Android KeyStore alias enumerations. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent b7575a1 commit d1b43a3

2 files changed

Lines changed: 30 additions & 22 deletions

File tree

src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidCertificatePal.cs

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -575,17 +575,14 @@ public void AppendPrivateKeyInfo(StringBuilder sb)
575575

576576
public void Dispose()
577577
{
578-
if (_privateKey != null)
579-
{
580-
_privateKey.Dispose();
581-
_privateKey = null;
582-
}
578+
_privateKey?.Dispose();
579+
_privateKey = null;
583580

584-
if (_cert != null)
585-
{
586-
_cert.Dispose();
587-
_cert = null!;
588-
}
581+
_keyStorePrivateKeyEntry?.Dispose();
582+
_keyStorePrivateKeyEntry = null;
583+
584+
_cert?.Dispose();
585+
_cert = null!;
589586
}
590587

591588
public byte[] Export(X509ContentType contentType, SafePasswordHandle password)

src/native/libs/System.Security.Cryptography.Native.Android/pal_x509store.c

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,7 @@ EnumerateCertificates(JNIEnv* env, jobject /*KeyStore*/ store, EnumCertificatesC
273273
// }
274274
// }
275275
jboolean hasNext = (*env)->CallBooleanMethod(env, aliases, g_EnumerationHasMoreElements);
276+
ON_EXCEPTION_PRINT_AND_GOTO(cleanup);
276277
while (hasNext)
277278
{
278279
INIT_LOCALS(loc, alias, entry, cert, publicKey, privateKey);
@@ -311,6 +312,7 @@ EnumerateCertificates(JNIEnv* env, jobject /*KeyStore*/ store, EnumCertificatesC
311312
RELEASE_LOCALS(loc, env);
312313

313314
hasNext = (*env)->CallBooleanMethod(env, aliases, g_EnumerationHasMoreElements);
315+
ON_EXCEPTION_PRINT_AND_GOTO(cleanup);
314316
}
315317

316318
ret = SUCCESS;
@@ -365,31 +367,34 @@ ARGS_NON_NULL_ALL static int32_t EnumerateTrustedCertificates(
365367
// }
366368
// }
367369
jboolean hasNext = (*env)->CallBooleanMethod(env, aliases, g_EnumerationHasMoreElements);
370+
ON_EXCEPTION_PRINT_AND_GOTO(cleanup);
368371
while (hasNext)
369372
{
373+
jobject cert = NULL;
370374
jstring alias = (*env)->CallObjectMethod(env, aliases, g_EnumerationNextElement);
371375
ON_EXCEPTION_PRINT_AND_GOTO(loop_cleanup);
372376

373377
if (filter == NULL || filter(env, alias))
374378
{
375-
jobject cert = (*env)->CallObjectMethod(env, store, g_KeyStoreGetCertificate, alias);
376-
if (cert != NULL && !CheckJNIExceptions(env))
379+
cert = (*env)->CallObjectMethod(env, store, g_KeyStoreGetCertificate, alias);
380+
if (!CheckJNIExceptions(env) && cert != NULL)
377381
{
378-
cert = ToGRef(env, cert);
379-
cb(cert, context);
382+
cb(AddGRef(env, cert), context);
380383
}
381384
}
382385

383-
hasNext = (*env)->CallBooleanMethod(env, aliases, g_EnumerationHasMoreElements);
384-
385386
loop_cleanup:
386-
(*env)->DeleteLocalRef(env, alias);
387+
ReleaseLRef(env, cert);
388+
ReleaseLRef(env, alias);
389+
390+
hasNext = (*env)->CallBooleanMethod(env, aliases, g_EnumerationHasMoreElements);
391+
ON_EXCEPTION_PRINT_AND_GOTO(cleanup);
387392
}
388393

389394
ret = SUCCESS;
390395

391396
cleanup:
392-
(*env)->DeleteLocalRef(env, aliases);
397+
ReleaseLRef(env, aliases);
393398
return ret;
394399
}
395400

@@ -431,9 +436,11 @@ jobject /*KeyStore*/ AndroidCryptoNative_X509StoreOpenDefault(void)
431436
(*env)->CallVoidMethod(env, store, g_KeyStoreLoad, NULL, NULL);
432437
ON_EXCEPTION_PRINT_AND_GOTO(cleanup);
433438
ret = ToGRef(env, store);
439+
store = NULL;
434440

435441
cleanup:
436-
(*env)->DeleteLocalRef(env, storeType);
442+
ReleaseLRef(env, store);
443+
ReleaseLRef(env, storeType);
437444
return ret;
438445
}
439446

@@ -444,19 +451,23 @@ int32_t AndroidCryptoNative_X509StoreRemoveCertificate(jobject /*KeyStore*/ stor
444451
abort_if_invalid_pointer_argument (store);
445452

446453
JNIEnv* env = GetJNIEnv();
454+
int32_t ret = FAIL;
447455

448456
jstring alias = make_java_string(env, hashString);
449457
if (!ContainsMatchingCertificateForAlias(env, store, cert, alias))
450458
{
451459
// Certificate is not in store - nothing to do
452-
return SUCCESS;
460+
ret = SUCCESS;
461+
goto cleanup;
453462
}
454463

455464
// store.deleteEntry(alias);
456465
(*env)->CallVoidMethod(env, store, g_KeyStoreDeleteEntry, alias);
466+
ret = CheckJNIExceptions(env) ? FAIL : SUCCESS;
457467

458-
(*env)->DeleteLocalRef(env, alias);
459-
return CheckJNIExceptions(env) ? FAIL : SUCCESS;
468+
cleanup:
469+
ReleaseLRef(env, alias);
470+
return ret;
460471
}
461472

462473
jobject AndroidCryptoNative_X509StoreGetPrivateKeyEntry(jobject /*KeyStore*/ store, const char* hashString)

0 commit comments

Comments
 (0)