Skip to content

Guard primitive array value-manager routing - #12114

Merged
simonrozsival merged 5 commits into
mainfrom
dev/simonrozsival/fix-valuemanager-array-routing
Aug 25, 2026
Merged

Guard primitive array value-manager routing#12114
simonrozsival merged 5 commits into
mainfrom
dev/simonrozsival/fix-valuemanager-array-routing

Conversation

@simonrozsival

@simonrozsivalsimonrozsival commented Jul 15, 2026

Copy link
Copy Markdown
Member

Summary

  • require the JNI runtime type to match the selected primitive-array marshaler before invoking array JNI APIs
  • route non-array IList<T> handles through JavaConvert only when JNI confirms the object implements java.util.List
  • apply the equivalent exact JNI signature and List-assignability guards to the trimmable PrimitiveArrayInfo path
  • honor JniObjectReferenceOptions before all new invalid-cast failures

CheckJNI evidence and safety boundary

The reproducer passed a java.util.ArrayList handle while requesting IList<bool>. The reflection value manager selected JavaBooleanArray.ArrayMarshaler from the managed target type alone, leading CheckJNI to abort the process with jarray argument has non-array type: java.util.ArrayList. This change establishes the safety boundary before any array JNI call: only the matching primitive array signature (for example, [Z) may enter that marshaler. A non-array reference may enter collection conversion only when JNI IsInstanceOf confirms java.util.List; other objects and mismatched arrays fail with InvalidCastException. Failure paths dispose and invalidate transferred references according to the requested ownership options.

Retained behavior

A genuine Java primitive array still marshals to IList<T>, and genuine Java List implementations still use JavaConvert. The guard compares the actual JNI runtime type rather than rejecting IList<T> generally.

Tests

Added on-device coverage in JavaConvertTest for:

  • java.util.ArrayList<Boolean> handle -> IList<bool>
  • Java boolean[] handle -> IList<bool>
  • Java String handle -> IList<bool> safely throws, preserving Copy and consuming CopyAndDispose
  • Java int[] handle -> IList<bool> safely throws and consumes CopyAndDispose

Validation:

  • dotnet build external/Java.Interop/src/Java.Interop/Java.Interop.csproj ... passed with 0 warnings/errors after the review fixes.
  • On-device tests were not run locally because this worktree has no prepared local SDK (make prepare is required).
  • A direct Mono.Android.csproj build reaches the repository prerequisite failure because bin/BuildDebug/net10.0/xa-prep-tasks.dll is absent.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0e043be9-9070-43bc-aea4-a48438690bdf
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0e043be9-9070-43bc-aea4-a48438690bdf
@simonrozsival

Copy link
Copy Markdown
MemberAuthor

@copilot resolve the merge conflicts in this pull request

Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>

CopilotAI commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Resolved the merge conflict with origin/main in commit 3b58d52b. The resolution keeps the primitive-array ValueManager regression tests and the new upstream FromJniHandle collection tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@simonrozsival
simonrozsival marked this pull request as ready for review August 21, 2026 10:31
CopilotAI lite review requested due to automatic review settings August 21, 2026 10:31

CopilotAI 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

Adds defensive JNI type guards around primitive-array marshaling so IList<T> conversions don’t accidentally invoke array JNI APIs on non-array objects (avoiding CheckJNI aborts), while still allowing java.util.List objects to be converted via JavaConvert when appropriate.

Changes:

  • In ReflectionJniValueManager, gate primitive-array marshalers on the actual JNI runtime type signature (e.g. only [Z may enter the boolean-array marshaler), and route non-array IList<T> conversions via a new CreateNonArrayListValue hook only when JNI confirms java.util.List.
  • Apply equivalent guards to the trimmable typemap path (PrimitiveArrayInfo).
  • Add on-device regression tests covering valid conversions and invalid-cast/ownership behavior for IList<bool> scenarios.
Show a summary per file
FileDescription
tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JavaConvertTest.csAdds on-device regression tests for list/primitive-array IList<bool> conversions and failure/ownership behaviors.
src/Mono.Android/Microsoft.Android.Runtime/PrimitiveArrayInfo.csAdds JNI runtime-type guard for primitive array wrappers and allows java.util.List to fall back to JavaConvert for IList<T>.
src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.csOverrides new CreateNonArrayListValue hook to route guarded non-array list conversions through JavaConvert.
src/Mono.Android/Android.Runtime/AndroidRuntime.csSame override as above for the runtime value manager used by AndroidRuntime.
external/Java.Interop/src/Java.Interop/PublicAPI.Unshipped.txtDeclares the newly added virtual ReflectionJniValueManager.CreateNonArrayListValue(...) API.
external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.ReflectionJniValueManager.csImplements primitive-array signature checks, java.util.List assignability gate, and new CreateNonArrayListValue extensibility point.

Review details

  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment on lines +187 to +190
var reference = new JniObjectReference (JNIEnv.NewArray (new [] { true, false }), JniObjectReferenceType.Local);
var converted = JniEnvironment.Runtime.ValueManager.GetValue<IList<bool>> (ref reference, JniObjectReferenceOptions.CopyAndDispose);

CollectionAssert.AreEqual (new [] { true, false }, converted);
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6d6744e2-1156-4eb8-b63e-cdb9102cd834
@simonrozsival

Copy link
Copy Markdown
MemberAuthor

/review

@simonrozsivalsimonrozsival added the ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable). label Aug 25, 2026
@github-actions

github-actionsBot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Android PR Reviewer completed successfully!

Generated by Android PR Reviewer for #12114

@github-actionsgithub-actionsBot 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.

⚠️ Needs Changes

Findings: 1 error · 0 warnings · 0 suggestions

The new runtime checks correctly protect the intended IList<T> conversion and preserve ownership on rejection, but the reflection value-manager path still treats managed array targets as list targets. A java.util.List requested as bool[] therefore reaches JNIEnv.GetArray through JavaConvert instead of failing safely. The trimmable path already avoids this by restricting its fallback to an actual IList<> target.

CI build #1564780 is still running; all completed checks currently pass.

Generated by Android PR Reviewer for #12114 · gpt56 · 137.5 AIC · ⌖ 10.8 AIC · ⊞ 25.7K
Comment /review to run again

if (IsMatchingPrimitiveArray (marshaler, jniType))
return marshaler.CreateValue (ref reference, options, targetType);

if (GetListType (targetType) != null && jniType != null && !jniType.StartsWith ("[", StringComparison.Ordinal)) {

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.

🤖 ❌ JNI interopGetListType(targetType) is also non-null for managed arrays because bool[] implements IList<bool>. Consequently, asking for bool[] when the JNI object is a java.util.List enters this branch and CreateNonArrayListValue delegates to JavaConvert; its array converter then calls JNIEnv.GetArray on the non-array reference, preserving the fatal JNI misuse this guard is intended to prevent. Please exclude array targets from this fallback (and add the corresponding GetValue<bool[]> regression case) so this path throws safely instead.

Rule: Validate the runtime JNI type before array access

@simonrozsival
simonrozsival merged commit 7b0b406 into mainAug 25, 2026
44 checks passed
@simonrozsival
simonrozsival deleted the dev/simonrozsival/fix-valuemanager-array-routing branch August 25, 2026 09:02
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-reviewThis PR is ready to review/merge, I think any CI failures are just flaky (ignorable).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@simonrozsival@rolfbjarne