Repository navigation
[Unsafe] Remove unsafe managed-memory access from hashing - #13041
Conversation
Use bounded managed reads and flat table indexing in both legacy CRC64 helpers, retaining all table constants, the CRC-64-Jones algorithm and HashAlgorithm wrappers. Validate input ranges before changing state. Use bounded little-endian operations for Files length XOR and span-based UTF8 encoding in the trimmable typemap scanner. Remove unsafe permission only from the five tooling projects whose evaluated inputs are now safe. Preserve package references, framework targets and task-hosting behavior. This addresses the tooling portion of issue #11467 step 2. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@dalexsoto review |
There was a problem hiding this comment.
🟡 Changes recommended
Permanent regression tests are needed for hash compatibility, range validation, and incremental state behavior.
2 open findings
What changed in this PR
Removes unsafe managed-memory access from tooling hashes while preserving legacy CRC64 behavior and public APIs.
Changes:
- Replaces pointer-based CRC and length operations with bounded managed APIs.
- Uses safe span-based UTF-8 encoding for typemap names.
- Removes unnecessary unsafe compilation permissions from five tooling projects.
| File | Description |
|---|---|
src/Microsoft.Android.Sdk.TrimmableTypeMap/Scanner/ScannerHashingHelper.cs |
Uses safe UTF-8 span encoding and checked byte counts. |
src/Microsoft.Android.Sdk.TrimmableTypeMap/Microsoft.Android.Sdk.TrimmableTypeMap.csproj |
Removes unsafe compilation permission. |
src/Microsoft.Android.Sdk.ILLink/Microsoft.Android.Sdk.ILLink.csproj |
Removes unsafe compilation permission. |
src/Microsoft.Android.Build.BaseTasks/Microsoft.Android.Build.BaseTasks.csproj |
Removes unsafe compilation permission. |
src/Microsoft.Android.Build.BaseTasks/Files.cs |
Replaces unsafe length XOR with bounded little-endian operations. |
src/Microsoft.Android.Build.BaseTasks/Crc64Helper.cs |
Adds range validation and safe CRC reads/indexing. |
src/Microsoft.Android.Build.BaseTasks/Crc64.Table.cs |
Flattens the CRC lookup table. |
src/Microsoft.Android.Build.BaseTasks/Crc64.cs |
Removes the unnecessary unsafe modifier. |
external/Java.Interop/src/Java.Interop.Tools.TypeNameMappings/Java.Interop.Tools.TypeNameMappings.csproj |
Removes unsafe compilation permission. |
external/Java.Interop/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/Crc64Helper.cs |
Adds range validation and safe CRC reads/indexing. |
external/Java.Interop/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/Crc64.Table.cs |
Flattens the CRC lookup table. |
external/Java.Interop/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/Crc64.cs |
Removes the unnecessary unsafe modifier. |
external/Java.Interop/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers.csproj |
Removes unsafe compilation permission. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
dalexsoto
left a comment
dalexsoto
left a comment
There was a problem hiding this comment.
Completed the full 13-file correctness review, integration/compilation-source-contract pass, candidate-ledger resolution and final independent-blocker sweep. The bounded CRC reads and flat indexing preserve the original ordered tables, Jones variant, total-length XOR, public HashAlgorithm wrappers and valid offset/incremental behavior; rejected ranges are checked before state changes. The Files little-endian operation and scanner UTF-8 spans retain supported hash/name outputs and buffer ownership.
The five unsafe-permission removals are consistent with the committed, linked and relevant generated-source contracts and available APIs. This independent cleanup leaves packages, SDKs, TFMs, task-hosting and the dependency graph unchanged, avoiding the separate migration risks in draft #13036. The intentionally bounded permanent-test scope and disclosed local-validation limits are accepted; no high-confidence blocker remains.
Evidence is immutable source/producer-contract inspection and source-derived compatibility models, with current-head CI considered separately. No local compiler/product/device execution or completed green CI is claimed.
simonrozsival
changed the title
Remove unnecessary unsafe access to managed memory in the existing tooling without replacing the legacy CRC64 engine or changing its public APIs.
Both CRC64 helpers now use bounded
BitConverter.ToUInt64reads and ordinary flat table indexing. The original 2048 constants and their order are unchanged, as are the CRC-64-Jones polynomial, initial value, length XOR,HashAlgorithmwrappers, offsets, and incremental hashing behavior. Input ranges are checked before either state value changes.Files.XorLengthuses bounded little-endian reads/writes, matching our supported platforms. The typemap scanner uses the existing safe UTF8 span overloads and checked combined byte counts, preserving the:separator and the distinctscrc64/legacycrc64naming policies. RemoveAllowUnsafeBlocksonly from the five tooling projects whose evaluated compile inputs no longer need it; retain the permission in Mono.Android and other JNI/runtime projects.Related to #11467, step 2 (tooling portion only). This is an independent main-based PR, not a stack on #13036. There are no package, SDK, TFM, feed, task-hosting, or dependency-graph changes, no new CRC engine, no public API removals, and no new permanent test fixtures.
Local validation
macOS arm64, worktree-local .NET 10.0.100 for stable tooling, repository-pinned SDK 12.0.100-alpha.1.26480.102 for net11 consumers. No system SDK changes.
bin/bootstrap-dotnet/dotnet build src/Microsoft.Android.Build.BaseTasks/Microsoft.Android.Build.BaseTasks.csproj -v quietbin/bootstrap-dotnet/dotnet build external/Java.Interop/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers.csproj -v minimalbin/bootstrap-dotnet/dotnet build external/Java.Interop/src/Java.Interop.Tools.TypeNameMappings/Java.Interop.Tools.TypeNameMappings.csproj -v minimal./dotnet-local.sh build src/Microsoft.Android.Sdk.TrimmableTypeMap/Microsoft.Android.Sdk.TrimmableTypeMap.csproj -v minimal./dotnet-local.sh build src/Microsoft.Android.Sdk.ILLink/Microsoft.Android.Sdk.ILLink.csproj --no-restore -v quietbin/bootstrap-dotnet/dotnet test tests/Microsoft.Android.Build.BaseTasks-Tests/Microsoft.Android.Build.BaseTasks-Tests.csproj --no-restore -v quietbin/bootstrap-dotnet/dotnet test external/Java.Interop/tests/Java.Interop.Tools.JavaCallableWrappers-Tests/Java.Interop.Tools.JavaCallableWrappers-Tests.csproj -v minimal --no-restore./dotnet-local.sh build src/Microsoft.Android.Build.Tasks/Tests/Microsoft.Android.Build.Tasks.Tests/Microsoft.Android.Build.Tasks.Tests.csproj -v minimalExisting typemap regression tests:
49 passed, 0 skipped, after Mono.Android was built.
A temporary, untracked comparison harness against original main assemblies passed 628 compatibility/state checks on .NET 10.0.0 and 726 on .NET 11 RC2, covering empty/zero/patterned inputs, offsets, incremental hashing, Initialize, Files byte/file/stream hashes, and empty/Unicode/surrogate/long scanner names. Invalid helper ranges reject without changing CRC or length. Both tables were compared against main for exact constant/order equality. No new tests are committed.
Evaluated compile inputs were checked for all five projects (19/29/10/54/27 inputs respectively); each compiles without unsafe permission. Fresh deployed BaseTasks and shared
tools/System.IO.Hashing.dllmatch their build outputs byte-for-byte; the latter also exactly matches the unchanged pinned 10.0.12 netstandard2.0 package asset, rather than a modern CLI asset overwriting it.This is a safety cleanup, not a zero-cost/performance claim. Temporary optimized kernel samples on this machine measured approximately +2.6% (64 bytes) and +5.4% (4 KiB) on .NET 10, with different results on .NET 11 RC2. Bounds checks remain; measured ComputeHash allocations were unchanged. These are local samples, not cross-platform guarantees.
Validation limitations
make prepareinitially failed restoring unpublishedMicrosoft.NETCore.App.Ref 10.0.13. Retrying with SDK 10 selected for both PATH andDotNetPreviewToolpassed bootstrap provisioning, but the API-docs submodule transfer stalled. JDK discovery was run separately with the actual repository task to allow consumer builds.make allcompiled the affected consumers, then failed during reference-pack packaging:CreateFrameworkListFilecould not findMicrosoft.Android/37/Mono.Android.xml. The local workload was therefore not fully configured. Full SDK packaging, extra API levels 37.1/37.2, and hostChangePackageNamingPolicyCoreCLR/NativeAOT build tests remain unvalidated. Windows/Linux and on-device execution were not run. The PR is opened now at the user's explicit request with these gaps disclosed.Pull Request
title and
description
should follow the
commit-messages.mdworkflow documentation, and in particular should include: