Repository navigation
[Unsafe][Java.Interop] Prepare managed peer construction - #13183
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
Cross-runtime JNI activation warrants human review because reported device validation covers the combined stack rather than this exact tip.
No open issues 🎉
What changed in this PR
Prepares managed JNI peer construction for updated C# memory-safety rules without enabling the compiler feature, supporting #11467 and the later #13070 migration.
Changes:
- Replaces null-pointer constructor sentinels with local references and forwarding constructors.
- Preserves exception stack capture for preinstalled peers and adds construction regressions.
- Rejects zero JNI environment pointers before native access or environment updates.
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/ConstructorActivationTests.cs | Tests Java- and managed-origin exception construction. |
| tests/Mono.Android-Tests/Java.Interop-Tests/Java.Interop/JniEnvironmentTests.cs | Tests zero-pointer rejection and environment preservation. |
| src/Mono.Android/Java.Lang/Object.cs | Uses the options-only base constructor. |
| external/Java.Interop/src/Java.Interop/PublicAPI.Unshipped.txt | Records the new protected constructor. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.cs | Guards against a zero environment pointer. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniEnvironment.cs | Adds zero-pointer guards. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JavaPrimitiveArrays.tt | Updates primitive-array constructor generation. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JavaPrimitiveArrays.cs | Updates all eight generated length constructors. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JavaObjectArray.cs | Removes the constructor sentinel argument. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JavaObject.cs | Adds options-only forwarding and local unsafe blocks. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JavaException.cs | Replaces sentinels and captures preinstalled-peer stacks. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JavaArray.cs | Adds internal options-only constructor chains. |
Review effort: Balanced
986a13f to
fefa427
Compare
Keep unsafe scopes around the actual JNI operations and preserve the existing fixed statement indentation. Shorten SAFETY comments without changing callback contracts or native-call ordering. Retain only the new regression for the actual baseline bug: successful unregistration held the original array and callback while the owner remained alive. Remove generic success and preparation coverage that already passed before this PR, preserving the complete earlier fixture in session recovery artifacts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace null-reference constructor sentinels with local default JNI references and forwarding constructors. Preserve Java stack capture when native activation has already installed an exception peer. Cover Java-origin and managed exception construction through the real constructor activation callbacks. Reject zero JNI environments before native dispatch without activating the updated memory-safety rules. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep default object/exception and length-based array construction safe for callers under the updated memory-safety rules. The remaining unsafe modifiers describe implementation details, not caller obligations. Scope native object creation inside explicit unsafe blocks, and remove the unnecessary modifiers from array constructors. Regenerate primitive arrays from the canonical template. Preserve exception stack capture for preinstalled peers, initialization order, and existing API records. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Record why managed-only construction remains safe for callers while raw JNI environment and handle entry points retain caller obligations. Document ABI, current-thread lifetime, aliasing, transfer and failure requirements without promising cleanup the implementation does not do. Add concrete SAFETY proofs to audited constructor and test boundaries. Regenerate the primitive-array output from its canonical template. Keep executable behavior, allocations, accepted calls and APIs intact. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove the added ordinary managed-construction parity test. It does not demonstrate a separate bug fixed by the unsafe audit. Retain the Java-origin preinstalled-peer stack capture regression and the null-JNIEnv transition regression. Preserve existing baseline tests and all production construction and ownership behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Follow the requested constructor layout: keep the public parameterless constructor first and delegate its no-reference initialization to a private options constructor below it. Preserve the original early return and capture the preinstalled peer stack in the private constructor, matching the original delegating reference constructor. Fresh-peer construction keeps its existing JNI order and stack capture. Each constructor has one capture site. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
fefa427 to
34ca9d9
Compare
simonrozsival
added
the
ready-to-review
|
@dalexsoto review |
|
/review |
|
✅ Android PR Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The constructor refactoring consistently replaces the invalid-reference sentinel with local default references, keeps unsafe JNI operations narrowly scoped, and adds focused coverage for preinstalled-peer stack capture and null JNIEnv* rejection. The remaining concern is the live diff’s loss of independent native-registration ownership and failure-path tests; please restore them or synchronize the parent branch so that reduction leaves this comparison.
CI is still in progress: the two completed Android Tools test checks and CLA are passing, while the Linux, macOS, Windows, and aggregate dotnet-android checks remain active.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
azcliprod.blob.core.windows.net
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
Generated by Android PR Reviewer for #13183 · copilot · gpt56 · 230.1 AIC · ⌖ 7.06 AIC · ⊞ 28.4K
Comment /review to run again
dalexsoto
left a comment
dalexsoto
left a comment
There was a problem hiding this comment.
The options-only constructor chain preserves caller-safe construction, peer ownership and single exception-stack capture; template/API updates and the null-environment guards match those paths. I treated the acknowledged lower-layer registration-test reduction as intentional scope, not restored/equivalent coverage or a proven production blocker.
Static review of the full14-file comparison against d37c215a78bab4f2a845f30c9c04573cee712edf; runtime, merged-SDK and compiler-feature validation are not certified.
Restore the registration fixture exactly from the current PR base. The callback preparation layer has merged, so these are existing main regressions rather than new unsafe-annotation coverage. Retain invocation, owner retention, disposal, failure propagation, retry, and independent-owner cases. Avoid deleting this behavioral coverage as part of managed constructor preparation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
❌ Android PR Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
@dalexsoto review |
|
@simonrozsival Not reviewing this request in this iteration: the full 13-file static review of head |
|
@dalexsoto review |
dalexsoto
left a comment
dalexsoto
left a comment
There was a problem hiding this comment.
The options-only constructor chain preserves caller-safe construction, peer ownership and single exception-stack capture; all eight registration cases remain restored. The complete 13-file source audit plus current-main consumer/SDK integration and a separate final blocker pass found no remaining source blocker.
The later Debug/Release/NoAab CI retry passed both focused regressions and the formerly failing GC/upload cases, superseding the earlier runtime-failure deferral. Those results test f1231b6d (44d713 + 945c), not current main. Current-target review is static per-path source composition against 2b6f3; the existing merge ref is stale (3c6b94 + 945c). No current-main merged execution, installed-SDK or shipping-artifact certification is claimed.
Related: #11467 (not closed by this PR). Preparation work originally extracted from historical #13070, source
951fa3a2dc588e15ff63eea4565d1dd14a30f036.Published state and temporary parent mismatch
Published head:
fefa427f8a8077418e60691c2031089b01e220c1. The base branchsimonrozsival-jni-callback-ownershipis still published at0074a9bb763f0cad7a9cf1ac88c18c9436cba5d9. Against that live base, GitHub currently shows 6 commits, 14 files, +207/-239.The head includes the prepared callback reduction
3b5cd38e5226bf59c61661a547d722574ae8dfdf, which has not been pushed to the parent branch. Consequently this PR temporarily also includes that lower layer's two-file churn/test reduction (eight registration cases reduced to one bug regression). The construction-only slice,3b5cd38e..fefa427f, is 5 commits, 12 files, +195/-32 (+189/-26 ignoring whitespace). It is not yet the complete live PR comparison. No parent/upper branch or stack metadata was changed by this branch's push; remaining stack synchronization is coordinated separately.Scope
Replace
ref *InvalidJniObjectReferenceconstructor sentinels with local default references and the options-only forwarding chain without invoking the allocating default base constructor. Keep all 11 managed constructors caller-safe: parameterlessJava.Interop.JavaObjectandJava.Interop.JavaException,Java.Interop.JavaObjectArray<T>(int), and eight primitive-array length constructors. Pointer/native initialization stays inside explicit local unsafe blocks. KeepJavaPrimitiveArrays.ttand its canonically regenerated.cstogether.The audit adds concrete
// SAFETY:proofs and documents actual raw-JNI ABI, thread, lifetime, aliasing, transfer, and failure obligations while preserving genuine caller-unsafe declarations. It does not redesign ownership, change allocations, or introduce new accepted-call restrictions.User-directed constructor layout: the public parameterless
JavaExceptionremains first and delegates to a private options constructor below it. Its originalif (PeerReference.IsValid) return;is preserved. The private constructor conditionally captures an already-installed peer's Java stack; fresh creation keeps its existing JNI sequence and capture. Each constructor has one capture site and each path captures once. This private forwarding change was explicitly requested in review; it adds no public API.Separate bug coverage: the pinned extraction source omitted stack capture on the preinstalled-peer path; preserving the original delegating-constructor behavior avoids that regression.
JavaSideExceptionConstructorCapturesStackFromPreinstalledPeerexercises real Java-origin activation.Transition_RejectsNullEnvironmentPointercovers zero-JNIEnv rejection before TLS mutation/native dispatch. These are the only two new construction bug tests; the added ordinary managed-construction parity test was removed. Existing baseline tests remain intact.Featuresstays off in this preparation layer. Final Java.Interop-only compiler activation/metadata assertions are in #13192. Mono.Android product feature activation is outside this stack.Validation and provenance
Latest checks cover the exact source now committed as
fefa427f, including the final review-requested constructor variant. JavaException source SHA-256 was pinned through compilation and commit:8bf969066a1a70aef11011dd0ed094dfd581b56f9c5c7385424b7030179e744f.With the existing local SDK,
DOTNET_ROOT="$PWD/bin/Debug/dotnet",DOTNET_ROLL_FORWARD=Major,DOTNET_PROCESSOR_COUNT=2,CMAKE_BUILD_PARALLEL_LEVEL=2,DOTNET_CLI_DO_NOT_USE_MSBUILD_SERVER=1, andMSBUILDDISABLENODEREUSE=1:./dotnet-local.sh build external/Java.Interop/src/Java.Interop/Java.Interop.csproj -c Debug --no-restore -p:DotNetTargetFrameworkVersion=10.0 -m:1 -nr:false -p:UseSharedCompilation=false -v:minimal -bl:bin/BuildDebug/construction-userreview-net10.binlog— passed, 0 warnings / 0 errors../dotnet-local.sh build external/Java.Interop/src/Java.Interop/Java.Interop.csproj -c Debug --no-restore -p:DotNetTargetFrameworkVersion=11.0 -m:1 -nr:false -p:UseSharedCompilation=false -v:minimal -bl:bin/BuildDebug/construction-userreview-net11.binlog— passed, 0 warnings / 0 errors.Each framework's first no-restore attempt failed
NETSDK1005because its target was absent from the current assets file. A cache-only restore of the core/direct generator closure using an empty feed and globally emptyRestoreAdditionalProjectSourcesresolved it; no network, bootstrap, or SDK installation was used. Both successful build commands above remained--no-restore.Existing Android fixture sources were compiled directly, without project build hooks or JNI execution. With
$cscdenoting the existingbin/Debug/dotnet/sdk/12.0.100-alpha.1.26509.101/Roslyn/bincore/cscand$evidencethe retained session artifact directory:"$csc" /noconfig @"$evidence/construction-reduced-jni-csc.rsp"— passed, 43 existing JNI fixture sources, 0 warnings / 0 errors."$csc" /noconfig @"$evidence/construction-reduced-runtime-csc.rsp"— passed, 91 existing CoreCLR/android-arm64 fixture sources, 0 warnings / 0 errors.Both use the freshly built Java.Interop reference, not a stale SDK-pack reference; its unchanged SHA-256 is
08fa33ecf501d39dfc44e2751bc29b0621365818c4a66e88907ad0a71669dc37. Actual Csc inputs/source hashes are retained. The private overload leaves public reference-assembly hashes unchanged. These are compilation results, not runtime test passes.Canonical T4 generation and
cmppassed on the reduced source; the final review follow-up changes only JavaException.git diff --check 3b5cd38e5226bf59c61661a547d722574ae8dfdf HEADalso passed after the final commit.Earlier SDK and four full runtime-suite receipts belong only to the historical original-parent
31001bbbthrough finalcd9e12f2stack union; retained commands/results are linked from #13192. They are not validation of the reduced root, final constructor follow-up, or fully revised stack. Historical standalone APKs retain their original provenance. No new combined updated-rule/full-SDK/device validation or CI-green status is claimed here.Pull Request
title and
description
should follow the
commit-messages.mdworkflow documentation, and in particular should include: