[r8-obfuscation] Add R8 JNI mapping primitives - #12628
Conversation
simonrozsival
changed the title
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
src/Xamarin.Android.Build.Tasks/Utilities/JniRemapping/R8Mapping.cs — CreateManifestContent() uses StringWriter.WriteLine(), which… |
 |
src/Xamarin.Android.Build.Tasks/Utilities/JniRemapping/JniDescriptorText.cs — JniTypeTokenToJavaSource() uses System.Linq.Enumerable.Repeat() to append… |
|
src/Xamarin.Android.Build.Tasks/Utilities/JniRemapping/R8Mapping.cs — 💡 suggestion codebase_conventions — New files in this repo typically use file-scoped namespaces… |
What changed in this PR
Adds the first “primitive” layer for opt-in R8 JNI name obfuscation by introducing self-contained utilities under Xamarin.Android.Build.Tasks for parsing R8 mapping.txt, validating/rewriting JNI descriptors, and rewriting JNI-sensitive ldstr string forms, along with focused NUnit coverage. This establishes the core mapping/rewriting building blocks so later stack layers can integrate them into the build without changing behavior in this PR.
Changes:
- Introduces
R8Mappingto parsemapping.txt, support forward/reverse lookups, and generate/consume compact manifest entries. - Adds
JniDescriptorTextandLdstrRewriterto validate/rewrite JNI descriptors and JNI-name-bearingldstrpayloads. - Adds unit tests covering mapping parsing/ambiguity/conflicts and descriptor/
ldstrrewriting behavior.
| File | Description |
|---|---|
| src/Xamarin.Android.Build.Tasks/Utilities/JniRemapping/R8Mapping.cs | R8/ProGuard mapping parser + lookup APIs + manifest/conflict helpers. |
| src/Xamarin.Android.Build.Tasks/Utilities/JniRemapping/JniDescriptorText.cs | JNI descriptor/token validation and embedded-class-name rewriting helpers. |
| src/Xamarin.Android.Build.Tasks/Utilities/JniRemapping/LdstrRewriter.cs | Rewrites known JNI-sensitive ldstr string forms using an IJniNameMapping. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/JniRemapping/R8MappingTests.cs | NUnit tests for mapping parsing, lookups, ambiguity, and conflict detection. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/JniRemapping/JniDescriptorTextTests.cs | NUnit tests for descriptor validation, rewriting, and Java-source conversion. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/JniRemapping/LdstrRewriterTests.cs | NUnit tests for rewriting ldstr-encoded member IDs, descriptors, and registration blocks. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Reject
Findings: 1 error, 0 warnings.
The parser and descriptor coverage are thorough, but reverse member mapping currently chooses an arbitrary original name for ambiguous R8 mappings, despite the PR’s stated ambiguity-safety guarantee. That can rewrite JNI member IDs to the wrong managed member; reverse field and name-only method lookups should fail closed when more than one candidate remains.
CI build 1576902 is still in progress with no failures reported at review time; the completed Android Tools macOS lane and CLA check passed.
Generated by Android PR Reviewer for #12628 · gpt56 · 111.5 AIC · ⌖ 8.88 AIC · ⊞ 25.7K
Comment /review to run again
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
Found 2 ❌ errors in the R8 parser: reverse class lookup is unsafe when class merging produces duplicate obfuscated names, and same-class inline frames can be indexed as live callable methods. Both can make the later rewrite/reachability pipeline accept an invalid mapping; details and regression-test guidance are inline.
The primitive boundaries, fail-closed member ambiguity handling, manifest snapshots, and focused test organization are otherwise strong. CI build #1576969 is still in progress: five dotnet-android lanes and CLA have passed, Windows/Linux builds are running, and no failure is currently reported.
Generated by Android PR Reviewer for #12628 · gpt56 · 174.9 AIC · ⌖ 9.02 AIC · ⊞ 25.7K
Comment /review to run again
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
simonrozsival
added
the
ready-to-review
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Require non-empty slash-separated JNI class names and reject Java source names or array markers embedded in object descriptors. Validate standalone type tokens before conversion so malformed input consistently throws ArgumentException. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fail reverse field and name-only method lookups when multiple allowed original names remain. Make manifest newlines deterministic and avoid LINQ allocation when formatting JNI array types. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Report source paths in mapping parse errors, make access tracking thread-safe with snapshot semantics, remove unused reverse lookup APIs, and avoid redundant JNI token validation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve every original class candidate for R8 class merging and disambiguate reverse mappings through the rewrite manifest. Parse positional mapping groups as retrace stacks so only the final residual frame is callable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
51b414b to
1263c29
Compare
There was a problem hiding this comment.
⚠️ Needs Changes
Found 1 warning in the reverse JNI method mapping: primitive descriptor types can be mistaken for residual class names when a custom/applied R8 mapping uses names such as int or void. The inline comment includes the required guard and regression-test shape.
The parser, ambiguity handling, access-manifest tracking, and focused test coverage are otherwise well structured. CI is green.
Generated by Android PR Reviewer for #12628 · gpt56 · 302.6 AIC · ⌖ 8.99 AIC · ⊞ 25.7K
Comment /review to run again
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Avoid treating Java primitive source names as package-less residual class names during reverse method lookup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
## Summary This is layer 2 of 6 in the replacement stack for PR #12575. It adds the internal PE and metadata reconstruction substrate required by later JNI name-rewriting layers, without enabling any build behavior. - Rebuilds managed PE images while preserving existing metadata tokens and supported table row ordering. - Re-emits method bodies, embedded resources, debug-directory data, native resources, and `FieldRVA` mapped data, including ranges from different PE sections. - Preserves overlapping/aliased `FieldRVA` backing data by relocating rewritten fields instead of modifying shared storage. - Validates malformed IL, resource offsets/directories, and strong-name signature directories through controlled rewrite errors. - Emits event/property method semantics explicitly in `HasSemantics` coded-index order. - Rejects unsupported field-backed `ImplMap` rows instead of silently dropping metadata. - Adds low-level metadata encoding, raw-column, IL operand-scanning, and rewrite-plan primitives. - Covers longer and shorter UTF-8 `FieldRVA` replacements, aliased mapped fields, event/property accessor preservation, per-use `#US` splitting, and strong-name signature preservation directly through `AssemblyRebuilder`. - Uses the executing runtime's core assembly version in generated fixtures so rebuilt images load across target frameworks. ## Scope This layer intentionally does **not** add JNI-specific planning or attribute/`ldstr` orchestration, the assembly-rewriter facade, the `RewriteJniNamesForR8` MSBuild task, typemap rewrite behavior, CoreCLR/NativeAOT target integration, or documentation. Those surfaces belong to later stack layers. ## Validation - `./dotnet-local.sh build src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Xamarin.Android.Build.Tests.csproj -c Debug -v:minimal --no-restore` - `./dotnet-local.sh test bin/TestDebug/net10.0/Xamarin.Android.Build.Tests.dll --filter 'FullyQualifiedName~NativeResourceSectionCopierTests|FullyQualifiedName~AssemblyRebuilderTests'` - Direct layer-2 result: 18 passed, 0 failed. - Cumulative layer-3 `RewrittenAssemblyLoadsAndRunsInTheRuntime` result: 1 passed, 0 failed. Related to #12535 Depends on #12628
Summary
Related to #12535
This is layer 1 of 6 in the replacement stack for PR #12575. It extracts the self-contained R8/JNI name-mapping primitives and their focused unit tests so later layers can add build integration and runtime-specific behavior independently.
This layer intentionally changes no build behavior. Nothing invokes these new helpers yet.
What this adds
R8Mapping\nnewlines.JniDescriptorText$separators when converting descriptors to R8 mapping keys.LdstrRewriterIJniNameMappingimplementation.RegisterNativesentries, multiline registration blocks, and exact JNI class-name strings.Test coverage
Adds focused tests for:
$descriptor-to-mapping-key conversion, and malformed standalone token handling.ldstrforms, including encoded member IDs, constructors, registration blocks, class names, unchanged inputs, and a forward/reverse field round trip restricted by the forward access manifest.Validation commands:
Result: 82 passed, 0 failed, 0 skipped; test project build succeeded with 0 warnings and 0 errors.
Intentionally out of scope
The following remain for later layers in the replacement stack:
FieldRVAhandling.The initial implementation and tests in this layer were reconstructed from the verified PR #12575 source snapshot at
ac2b30593ff5afa0659fe35d3de515239c7540fe, followed by descriptor-validation, ambiguity-safety, diagnostics, concurrency, merged-class, inline-frame, and owning-class access-tracking hardening from review. PR #12575 and its branch remain unchanged.