Remove unsafe code from CborHelpers, Crc32, and BlobUtilities

[EgorBo]

Note

This PR was generated with the assistance of GitHub Copilot.

Summary

Replace Unsafe.* and MemoryMarshal.* API usage with safe BinaryPrimitives equivalents in three libraries:

System.Formats.Cbor — CborHelpers.netstandard.cs

• ReadHalfBigEndian: MemoryMarshal.Read + manual endian swap → BinaryPrimitives.ReadUInt16BigEndian
• WriteHalfBigEndian: MemoryMarshal.Write + manual endian swap → BinaryPrimitives.WriteUInt16BigEndian
• ReadDoubleBigEndian: MemoryMarshal.Read + ReverseEndianness → BinaryPrimitives.ReadInt64BigEndian
• WriteDoubleBigEndian: MemoryMarshal.Write + ReverseEndianness → BinaryPrimitives.WriteInt64BigEndian

System.IO.Hashing — Crc32ParameterSet.WellKnown.cs

• UpdateScalarArm64, UpdateScalarArm: Unsafe.ReadUnaligned → BinaryPrimitives.ReadUInt64/32LittleEndian
• UpdateIntrinsic (SSE path): MemoryMarshal.Cast<byte, ulong/uint> → explicit for-loops with BinaryPrimitives
• UpdateIntrinsic (ARM path): Unsafe.ReadUnaligned → BinaryPrimitives.ReadUInt64/32LittleEndian
• Removed MemoryMarshal.GetReference + Unsafe.Add patterns
• Simplified remainder-byte loops

System.Reflection.Metadata — BlobUtilities.cs

• WriteUInt16/32/64 and BE variants: Unsafe.WriteUnaligned → BinaryPrimitives.Write*LittleEndian/BigEndian
• WriteDouble (#else branch): pointer cast *(ulong*)&value → BitConverter.DoubleToInt64Bits

All changes are behavioral equivalents — no public API changes. All existing tests pass.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR replaces several Unsafe.* / MemoryMarshal.*-based unaligned read/write patterns with System.Buffers.Binary.BinaryPrimitives (and BitConverter bit-conversion helpers) to keep the implementations safe while preserving behavior.

Changes:

• System.Reflection.Metadata: replace unaligned integer writes with BinaryPrimitives.Write*Endian and remove unsafe double bit reinterpretation in WriteDouble (non-NET path).
• System.IO.Hashing: replace Unsafe.ReadUnaligned / MemoryMarshal.Cast patterns in CRC32(C) intrinsic/scalar paths with BinaryPrimitives.Read*LittleEndian.
• System.Formats.Cbor: simplify big-endian half/double read/write helpers using BinaryPrimitives.Read/Write*BigEndian.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs	Switches unaligned integer writes to BinaryPrimitives and uses BitConverter.DoubleToInt64Bits for safe double bit extraction in non-NET builds.
src/libraries/System.IO.Hashing/src/System/IO/Hashing/Crc32ParameterSet.WellKnown.cs	Removes Unsafe/MemoryMarshal usage in CRC32(C) update loops, using BinaryPrimitives reads instead.
src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/CborHelpers.netstandard.cs	Replaces endian-swap + MemoryMarshal patterns for half/double big-endian helpers with BinaryPrimitives calls.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/CborHelpers.netstandard.cs:104

• PR description indicates CborHelpers replaces Unsafe.* and MemoryMarshal.* usage with BinaryPrimitives equivalents, but CborHelpers.netstandard.cs still uses MemoryMarshal.Read/Write (e.g., ReadSingleBigEndian/WriteSingleBigEndian) and contains unsafe pointer casts for float bit conversions. If the PR goal is to fully remove these APIs/unsafe code from CborHelpers, consider updating the remaining helpers (where possible for the target TFMs) or clarifying the scope in the PR description.

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static ushort ReadHalfBigEndian(ReadOnlySpan<byte> source)
        {
            return BinaryPrimitives.ReadUInt16BigEndian(source);
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static float ReadSingleBigEndian(ReadOnlySpan<byte> source)
        {
            return BitConverter.IsLittleEndian ?
                Int32BitsToSingle(BinaryPrimitives.ReverseEndianness(MemoryMarshal.Read<int>(source))) :
                MemoryMarshal.Read<float>(source);
        }

[jkotas]

Suggested change

	extension(BinaryPrimitives)
	{
	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	public static double ReadDoubleBigEndian(ReadOnlySpan<byte> source)
	{
	return BitConverter.Int64BitsToDouble(BinaryPrimitives.ReadInt64BigEndian(source);
	}
	}

Can we convert all polyfills in this type to extension methods like this, so that the actual source can use all modern APIs directly without unnecessary wrappers?

[jkotas]

Suggested change

	BinaryPrimitives.WriteInt64BigEndian(_buffer.AsSpan(_offset), BitConverter.DoubleToInt64Bits(value));
	BinaryPrimitives.WriteDoubleBigEndian(_buffer.AsSpan(_offset), value);

[jkotas]

Can we go further and delete this file completely?

[jkotas]

Suggested change

	return BitConverter.IsLittleEndian ?
	CborHelpers.Int32BitsToSingle(BinaryPrimitives.ReverseEndianness(MemoryMarshal.Read<int>(source))) :
	MemoryMarshal.Read<float>(source);
	return CborHelpers.Int32BitsToSingle(BinaryPrimitives.ReadInt32BigEndian(source));

[jkotas]

Same here

[jkotas]

Suggested change

	BinaryPrimitives.WriteSingleLittleEndian(buffer.AsSpan(start), value);

[jkotas]

Inline the unsafe casts into the BinaryPrimitives extensions directly?

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[jkotas]

I did not mean to add a bunch of ifdefs to the common file.

I meant to:

• Delete CborHelpers.netcoreapp.cs
• Use the netcoreapp APIs directly throughout the code
• Add extensions to CborHelpers.netstandard.cs to polyfill the netcoreapp APIs

[EgorBo]

Thanks, I did have that in mind, in my workflow I just ask AI to address the feedback and then change it. It sometimes proactively pushes it before I change it, but I assumed it's fine since the PR is in draft 🙂 Still, I appreciate the early feedback!

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/CborHelpers.cs:11

• using System.Buffers; and using System.Runtime.InteropServices; are not used anywhere in this file, which will produce unused-using warnings (treated as errors in this repo). Please remove the unused using directives.
  src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/CborHelpers.cs:179
• The BinaryPrimitivesPolyfills / BitConverterPolyfills blocks introduce new unsafe code and rely on extension(Type) syntax. This undermines the PR goal of removing unsafe code, and the extension-member syntax is not used elsewhere in the repo (making it risky/unclear for multi-target builds). Since BinaryPrimitives.Read/WriteSingle/Double*Endian already exist in System.Memory (used in netstandard builds), please remove these polyfills and call the existing BinaryPrimitives APIs directly; for bit conversions, keep the previous internal helper methods instead of extending BitConverter.

[Copilot]

On non-.NETCoreApp targets (netstandard2.0 / .NET Framework), BitConverter.SingleToInt32Bits is not available (the test helper for netstandard uses an unsafe bitcast for this reason). This change will break compilation unless you provide a valid polyfill; consider restoring the previous internal helper (e.g., CborHelpers.SingleToInt32Bits) or comparing bit patterns via BinaryPrimitives-based conversions instead.

[Copilot]

BitConverter.UInt32BitsToSingle, BitConverter.Int32BitsToSingle, and BitConverter.SingleToUInt32Bits are not available on the netstandard2.0 / .NET Framework TFMs that this file targets, so these replacements will fail to compile without a robust polyfill. Please keep using the existing internal conversion helpers (previously on CborHelpers) or implement local bit-conversion helpers for these TFMs.

[github-actions]

🤖 Copilot Code Review — PR #126269

Note

This review was generated by GitHub Copilot and has not been manually verified.

Holistic Assessment

Motivation: Well-justified. Replacing manual Unsafe.WriteUnaligned / MemoryMarshal.Read/Write + endian-swap patterns with BinaryPrimitives calls is the right direction. The reduce-unsafe label is appropriate. The Crc32 revert after stephentoub/jkotas feedback about bounds-check overhead was a good call.

Approach: Clean. The C# 14 extension(BinaryPrimitives) / extension(BitConverter) pattern for netstandard polyfills is exactly what jkotas requested — callers use the modern APIs directly (BinaryPrimitives.ReadDoubleBigEndian, BitConverter.SingleToInt32Bits) and the polyfills transparently fill the gap for older TFMs. Merging the two partial class files into a single CborHelpers.cs with #if NET / #else blocks is a net simplification.

Summary: ✅ LGTM. The changes are semantically equivalent, correctly constrained to internal helpers, and address all maintainer review feedback. No blocking issues found. Two minor observations below.

---

Detailed Findings

✅ Correctness — All BinaryPrimitives replacements are semantically equivalent

Verified each transformation:

• CborHelpers: MemoryMarshal.Read<ushort> + ReverseEndianness → BinaryPrimitives.ReadUInt16BigEndian — equivalent. All four Read/Write Half/Single/Double helpers are correct.
• BlobUtilities: Unsafe.WriteUnaligned(ref buffer[start], endian-swapped-value) → BinaryPrimitives.Write*LittleEndian/BigEndian(buffer.AsSpan(start), value) — equivalent and strictly safer (the old Unsafe.WriteUnaligned could silently write past array bounds if start was near the end).
• BlobUtilities WriteDouble #else: *(ulong*)&value → unchecked((ulong)BitConverter.DoubleToInt64Bits(value)) — bit-preserving. DoubleToInt64Bits is available on netstandard2.0.
• HalfHelpers.netstandard.cs: CborHelpers.UInt32BitsToSingle(...) → BitConverter.UInt32BitsToSingle(...) — resolves to the polyfill extension; correct.
• CborWriter.Simple.netstandard.cs: CborHelpers.SingleToInt32Bits(...) → BitConverter.SingleToInt32Bits(...) — resolves to the polyfill extension; correct.

✅ Extension polyfill design — Correct and consistent with maintainer guidance

The BinaryPrimitivesPolyfills and BitConverterPolyfills classes using extension(BinaryPrimitives) / extension(BitConverter) are properly guarded by #if !NET, so they only compile for netstandard2.0 / .NET Framework where the real APIs are missing. On modern .NET, the actual BCL methods are used directly with no wrapper overhead.

The remaining unsafe pointer casts in the polyfills (*(float*)&intValue, *(int*)&value) are necessary for netstandard2.0 where BitConverter.SingleToInt32Bits / Int32BitsToSingle don't exist. This is the right tradeoff — unsafe code is isolated to polyfills that only compile on older TFMs.

✅ File consolidation — CborHelpers merge is correct

Deleting CborHelpers.netcoreapp.cs and merging into CborHelpers.cs is clean. The .csproj correctly moves the <Compile> include from the TFM-conditional ItemGroup to the unconditional one. The class is no longer partial since there's only one file, which is correct.

✅ No public API surface changes

All changed types are internal. No ref/ assembly changes. No new public members.

💡 Unused using directives — Minor cleanup

using System.Runtime.InteropServices; appears unused in both CborHelpers.cs (line 11) and BlobUtilities.cs (line 8) now that all MemoryMarshal / Unsafe.WriteUnaligned calls have been removed. CI/analyzers should catch this, but noting it in case they don't.

Generated by Code Review for issue #126269 · ◷

[EgorBo]

I'll split this PR into smaller ones since it now does more than I initially wanted (to remove unsafe) also it is getting harder to review. Also, I should probably rely less on AI on addressing feedback 😥