Skip to content

[Android] Add missing JNI exception checks across crypto PAL - #130555

Merged
simonrozsival merged 6 commits into
mainfrom
dev/simonrozsival/android-pal-jni-exception-checks-pr2
Jul 17, 2026
Merged

[Android] Add missing JNI exception checks across crypto PAL#130555
simonrozsival merged 6 commits into
mainfrom
dev/simonrozsival/android-pal-jni-exception-checks-pr2

Conversation

@simonrozsival

@simonrozsival simonrozsival commented Jul 11, 2026

Copy link
Copy Markdown
Member

Note

This PR description was drafted by an AI assistant (GitHub Copilot CLI) based on the actual code changes in this branch. All code was authored interactively with Simon driving the design decisions.

Follow-up to #128777, which added the initial batch of JNI exception checks to the Android crypto PAL. This PR covers the remaining functions.

Summary

Adds missing JNI exception checks across the remaining functions in the Android cryptography PAL (src/native/libs/System.Security.Cryptography.Native.Android/). The work is based on a static audit of Call*Method / NewObject / Get*ArrayRegion-style JNI calls where a pending Java exception could otherwise leak back into managed code (undefined behavior for the next managed→native transition) or produce partial output.

Beyond adding the checks, the touched functions are tightened to follow three patterns already established elsewhere in the PAL:

  1. ON_EXCEPTION_PRINT_AND_GOTO(label) immediately after each fallible JNI call. The macro both prints and clears the exception, so subsequent JNI calls on the cleanup path are safe.
  2. INIT_LOCALS(loc, …) + a single cleanup:/RELEASE_LOCALS(loc, env) tail for local-ref hygiene, replacing ad-hoc jobject foo = NULL; declarations and scattered DeleteLocalRef calls.
  3. No partial output state on failure — results are computed into locals and only promoted to caller-visible out-parameters / global refs on the success path.

Native changes

All under src/native/libs/System.Security.Cryptography.Native.Android/:

File Functions
pal_x509chain.c X509ChainBuild, X509ChainGetCertificateCount, X509ChainGetCertificates, X509ChainGetErrorCount, PopulateValidationError, X509ChainGetErrors, X509ChainSetCustomTrustStore, CreateCertPathFromAnchor, ValidateWithRevocation, and the error-capture branches
pal_bignum.c BigNumToBinary, GetBigNumBytesIncludingPaddingByteForSign
pal_cipher.c CipherSetKeyAndIV, CipherUpdateAAD, CipherUpdate (also releases the update output array on all paths)
pal_ecdsa.c EcDsaSign (clear exception on the getPrivate() failure path)
pal_eckey.c NewEcKeyFromKeys, EcKeyCreateByOid (converted to INIT_LOCALS + per-call checks), EcKeyGetCurveName
pal_hmac.c DoFinal
pal_misc.c GetRandomBytes
pal_pbkdf2.c Pbkdf2 (clear the pending exception when NewDirectByteBuffer fails)
pal_sslstream.c SSLStreamSetTargetHost, SSLStreamRead, SSLStreamRequestClientAuthentication, SSLStreamSetApplicationProtocols
pal_x509.c X509Decode, X509DecodeCollection, X509GetContentType (check SetByteArrayRegion)

Managed changes

  • Interop.X509Chain.csX509ChainGetErrorCount now returns a negative value when a JNI exception occurs (distinct from a legitimate count of 0); the wrapper throws CryptographicException in that case instead of silently reporting "no errors" (which would leave ChainStatus empty for a failed validation).
  • Interop.Bignum.csAndroidCryptoNative_BigNumToBinary now returns -1 when a JNI exception occurs (the output must be treated as invalid — it may be zero-filled or only partially written); ExtractBignum throws CryptographicException rather than returning a corrupt buffer.

Testing

  • Native build: android-arm64 Release — 0 warnings / 0 errors.
  • Managed build: libs -os android -a arm64 -rf CoreCLR (includes System.Security.Cryptography) — 0 warnings / 0 errors.

CI will provide cross-architecture validation and device/emulator test runs.

Adds missing JNI exception checks across the remaining functions in the
Android cryptography PAL, based on a static audit of `Call*Method` /
`NewObject` / `Get*ArrayRegion`-style JNI calls where a pending Java
exception could otherwise leak back into managed code (undefined behavior
for the next managed->native transition) or produce partial output.

The touched functions follow the patterns already established elsewhere in
the PAL:

1. `ON_EXCEPTION_PRINT_AND_GOTO(label)` immediately after each fallible JNI
   call. The macro both prints and clears the exception, so subsequent JNI
   calls on the cleanup path are safe.
2. `INIT_LOCALS(loc, ...)` + a single `cleanup:`/`RELEASE_LOCALS(loc, env)`
   tail for local-ref hygiene, replacing ad-hoc `jobject foo = NULL;`
   declarations and scattered `DeleteLocalRef` calls.
3. No partial output state on failure -- results are computed into locals
   and only promoted to caller-visible out-parameters / global refs on the
   success path.

Files changed (all under
`src/native/libs/System.Security.Cryptography.Native.Android/` unless noted):

- `pal_x509chain.c` -- harden the functions not covered by the earlier
  `X509ChainCreateContext` fix (build, certificate/error enumeration,
  custom trust store, revocation validation) and clear exceptions on the
  error-capture paths.
- `pal_bignum.c` -- check `toByteArray()` before using the result.
- `pal_cipher.c` -- `CipherSetKeyAndIV`, `CipherUpdateAAD`, `CipherUpdate`
  (also releases the update output array on all paths).
- `pal_ecdsa.c` -- clear the exception on the `getPrivate()` failure path in
  `EcDsaSign`.
- `pal_eckey.c` -- `NewEcKeyFromKeys`, `EcKeyCreateByOid`
  (converted to `INIT_LOCALS` + per-call checks), `EcKeyGetCurveName`.
- `pal_hmac.c` -- check `Mac.doFinal()` before reading the result array.
- `pal_misc.c` -- `GetRandomBytes`.
- `pal_pbkdf2.c` -- clear the pending exception when `NewDirectByteBuffer`
  fails.
- `pal_sslstream.c` -- `SetTargetHost`, `SSLStreamRead`,
  `RequestClientAuthentication`, `SetApplicationProtocols`.
- `pal_x509.c` -- check `SetByteArrayRegion` in the decode paths.

Managed changes:

- `Interop.X509Chain.cs` -- `X509ChainGetErrorCount` now returns a negative
  value when a JNI exception occurs (distinct from a legitimate count of 0);
  the wrapper throws `CryptographicException` in that case instead of
  silently reporting "no errors".
- `Interop.Bignum.cs` -- `AndroidCryptoNative_BigNumToBinary` now returns -1
  when a JNI exception occurs and writes nothing; `ExtractBignum` throws
  `CryptographicException` rather than returning a zero-filled buffer.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: bc96caa6-f528-4662-8cfa-df04e8a47475

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the Android cryptography native PAL by adding missing JNI exception checks and tightening failure-path behavior so pending Java exceptions don’t leak into subsequent JNI transitions; it also updates managed interop to surface certain JNI failures as CryptographicException.

Changes:

  • Added ON_EXCEPTION_PRINT_AND_GOTO(...) / CheckJNIExceptions(...) checks after additional fallible JNI calls across the Android crypto PAL.
  • Adjusted native return conventions for some functions to better distinguish JNI-exception failures from legitimate “0 results”.
  • Updated managed Android interop wrappers to throw when native indicates a JNI exception occurred (e.g., negative error counts / negative bignum conversions).
Show a summary per file
File Description
src/native/libs/System.Security.Cryptography.Native.Android/pal_x509chain.c Adds JNI exception checks and tightens error materialization/cleanup when building/validating chains and collecting errors.
src/native/libs/System.Security.Cryptography.Native.Android/pal_x509.c Adds missing exception checks after byte-array population JNI calls.
src/native/libs/System.Security.Cryptography.Native.Android/pal_sslstream.c Adds exception checks around additional SSLEngine/ByteBuffer JNI operations to avoid leaking pending exceptions.
src/native/libs/System.Security.Cryptography.Native.Android/pal_pbkdf2.c Adds exception check after NewDirectByteBuffer and other JNI byte-buffer operations.
src/native/libs/System.Security.Cryptography.Native.Android/pal_misc.c Converts to a ret/cleanup pattern and adds some exception checks in GetRandomBytes.
src/native/libs/System.Security.Cryptography.Native.Android/pal_hmac.c Adds exception checks in the HMAC finalization path to prevent pending exceptions from escaping.
src/native/libs/System.Security.Cryptography.Native.Android/pal_eckey.c Improves JNI exception handling for EC key creation and curve name extraction.
src/native/libs/System.Security.Cryptography.Native.Android/pal_ecdsa.c Ensures exception state is cleared/observed on getPrivate() failure path.
src/native/libs/System.Security.Cryptography.Native.Android/pal_cipher.c Adds exception checks in cipher update/AAD paths and ensures local refs are released on all paths.
src/native/libs/System.Security.Cryptography.Native.Android/pal_bignum.c Adds exception checks and returns a negative result on JNI failure for bignum-to-binary conversion.
src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.X509Chain.cs Throws when native indicates a JNI exception during chain error-count retrieval.
src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Bignum.cs Propagates native “JNI exception occurred” via CryptographicException instead of returning corrupt/zero-filled data.

Copilot's findings

  • Files reviewed: 12/12 changed files
  • Comments generated: 4

Comment thread src/native/libs/System.Security.Cryptography.Native.Android/pal_misc.c Outdated
Comment thread src/native/libs/System.Security.Cryptography.Native.Android/pal_x509chain.c Outdated
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to 'arch-android': @vitek-karas, @simonrozsival, @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

…uched crypto PAL functions

Refinement pass over the functions touched earlier in this PR:

- Convert manual `jobject foo = ...;` + `DeleteLocalRef` local-ref handling to
  `INIT_LOCALS(loc, ...)` + a single `cleanup:`/`RELEASE_LOCALS(loc, env)` tail
  in the touched functions (pal_bignum.c, pal_cipher.c, pal_ecdsa.c,
  pal_eckey.c, pal_hmac.c, pal_misc.c, pal_pbkdf2.c, pal_x509chain.c).
- In `AndroidCryptoNative_X509ChainGetCertificates`, the trust-anchor local ref
  now lives in the `INIT_LOCALS` array and the caller-owned global ref is
  produced with `AddGRef` (instead of `ToGRef`); `RELEASE_LOCALS` frees the
  local ref on all paths, which also fixes a local-ref leak on the branch where
  the trust anchor equalled the last path certificate.
- Ensure caller-visible out-parameters are zero-initialized at function entry
  and only assigned their meaningful value on the success path
  (`CipherUpdate`, `DoFinal`, `EcKeyGetCurveName`).

No functional change on the success paths. Native build (android-arm64
Release) is clean with 0 warnings / 0 errors.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: bc96caa6-f528-4662-8cfa-df04e8a47475
…-or-nothing

`AndroidCryptoNative_X509ChainGetCertificates` filled the caller's `certs`
array incrementally and, if a JNI call threw partway through, left the
already-created global references in the array and returned FAIL, relying on
the managed wrapper's `finally` to release those partial entries.

Track the number of entries stored and, on any failure, release the global
references we created and clear the slots, so the native function is
self-contained: the caller only ever observes a fully populated array on
success and never has to clean up partial native state on failure. Update the
managed wrapper comment accordingly (its `finally` now releases the
native-returned refs on success and is a no-op on failure).

Native build (android-arm64 Release) is clean with 0 warnings / 0 errors.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: bc96caa6-f528-4662-8cfa-df04e8a47475
- pal_misc: check for a pending JNI exception immediately after
  SetByteArrayRegion in GetRandomBytes
- pal_x509chain: move exception checks ahead of DeleteLocalRef, and store
  loop/temporary local refs (anchor, endOnly, cert) in the loc[] array so
  RELEASE_LOCALS frees them on every path; null out message after free in
  the X509ChainGetErrors failure rollback
- pal_sslstream: use INIT_LOCALS/RELEASE_LOCALS in SSLStreamRead instead of
  a raw local + manual ReleaseLRef
- pal_eckey: convert NewEcKeyFromKeys to the INIT_LOCALS/RELEASE_LOCALS
  cleanup pattern to avoid leaking the curveParameters local ref
- Interop.Bignum: reword the negative-result comment more conservatively

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e1f935e1-2a62-4d1b-94ed-88a01eafaa4b
Copilot AI review requested due to automatic review settings July 16, 2026 10:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 12/12 changed files
  • Comments generated: 2

…ArrayLength

Address review feedback:
- pal_x509chain (ValidateWithRevocation): check for a pending JNI exception
  immediately after GetStaticObjectField for the ONLY_END_ENTITY option, before
  calling CallBooleanMethod
- pal_hmac (DoFinal): check after GetArrayLength so we don't call
  GetByteArrayRegion with a pending exception

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e1f935e1-2a62-4d1b-94ed-88a01eafaa4b
Copilot AI review requested due to automatic review settings July 16, 2026 10:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 12/12 changed files
  • Comments generated: 1

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 16, 2026 19:56
@simonrozsival
simonrozsival marked this pull request as ready for review July 16, 2026 19:56
@simonrozsival

Copy link
Copy Markdown
Member Author

/azp run runtime-android

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 12/12 changed files
  • Comments generated: 2

@simonrozsival
simonrozsival merged commit c728a82 into main Jul 17, 2026
103 of 111 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/android-pal-jni-exception-checks-pr2 branch July 17, 2026 08:27
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 11.0-preview7 milestone Jul 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants