Use checked context for memcpy's like API invocations

[Copilot]

Validate that the length passed to Buffer.MemoryCopy, Buffer.BlockCopy, and Unsafe.Copy* doesn't silently overflow.

Created from Copilot CLI via the copilot delegate command.

---

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR strengthens overflow safety around memcpy-like operations by ensuring byte/size calculations don’t wrap silently before being passed to Buffer.MemoryCopy, Buffer.BlockCopy, or Unsafe.CopyBlock, aligning these call sites with the intended “fail fast on overflow” behavior.

Changes:

• Use checked arithmetic when computing allocation/copy sizes for JSImport signature buffers.
• Add debug-time assertions to validate copy bounds before MemoryCopy / BlockCopy.
• Use checked multiplication for TraceLogging array size computation.

Reviewed changes

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

File	Description
src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSHostImplementation.cs	Adds checked to signature buffer size calculations to prevent silent overflow before allocation/copy.
src/libraries/System.Private.CoreLib/src/System/Security/SecureString.cs	Adds a debug assertion ensuring the copy length fits within the destination buffer before Buffer.MemoryCopy.
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/TraceLogging/FieldMetadata.cs	Adds a debug-time bounds assertion around Buffer.BlockCopy of custom metadata.
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/TraceLogging/DataCollector.cs	Uses checked multiplication for array byte-size computation before buffering/copying.

[github-actions]

🤖 Copilot Code Review — PR #125759

Note

This review was generated by Copilot using Claude Opus 4.6.

Holistic Assessment

Motivation: Justified. Integer overflow in size computations for memory copy/allocation operations is a well-known class of security vulnerability. The checked() additions convert silent wraparound into explicit OverflowException, which is strictly better. The Debug.Assert additions provide early detection of invariant violations in debug builds. This is consistent with the codebase convention: "Guard integer arithmetic against overflow... We have a lot of scars from integer overflow security bugs" — jkotas.

Approach: Correct. checked() is the idiomatic .NET mechanism for guarding arithmetic that feeds allocation/copy sizes. Debug.Assert is appropriate for internal invariant validation where callers already maintain the invariant and runtime APIs provide release-build protection.

Summary: ✅ LGTM. All four changes are correct, low-risk defensive hardening with no behavioral change for valid inputs. No public API surface is modified. The checked() additions complete existing patterns in the same files (five other checked() calls already exist in DataCollector). Two minor observations below — neither is merge-blocking.

---

Detailed Findings

✅ Security — JSHostImplementation.GetMethodSignature is the highest-value fix

The old code computed the NativeMemory.Alloc size through three unchecked arithmetic expressions. With a pathologically long string (~1B+ characters), functionName.Length * 2 would silently wrap negative, causing under-allocation followed by out-of-bounds writes via Unsafe.CopyBlock. The new checked() expressions correctly cover all three risk points. Current callers pass null for both string parameters, so this is defense-in-depth, but the method is public static within the internal class and the API accepts arbitrary strings — protecting it is the right call.

The refactoring from three separate size += statements into consolidated checked(size + 4 + functionNameBytes) is also a readability improvement — it makes the total size computation easier to audit.

✅ Correctness — DataCollector.AddArray completes the checked() pattern

length is capped to ushort.MaxValue (65,535) and itemSize is a compile-time sizeof(T) (max 16 for Guid), so the product maxes at ~1 MB — overflow is unreachable in practice. However, the five other arithmetic operations in the same file already use checked() (lines 100, 135, 174, 215, 355), so this addition completes the pattern. Good consistency.

✅ Safety — SecureString.UnmanagedBuffer.Copy assertion is sound

Buffer.MemoryCopy validates bounds at runtime in Release builds, so the Debug.Assert is a development-time early indicator. Both callers maintain the invariant: the copy constructor uses str._buffer.ByteLength for both allocation and copy, and EnsureCapacity allocates a larger buffer before copying. _decryptedLength is bounded to MaxLength = 65,536, so the caller-side arithmetic (uint)_decryptedLength * sizeof(char) is safe (max 131,072).

✅ Correctness — FieldMetadata.Encode assertion validates bounds before BlockCopy

The assertion pos >= 0 && this.fixedCount >= 0 && pos <= metadata.Length - this.fixedCount correctly validates that the Buffer.BlockCopy destination range [pos, pos + fixedCount) fits within metadata. This properly addresses the earlier inline review suggestion to avoid checked() inside Debug.Assert.

💡 Observation — Tautological sub-condition in FieldMetadata assertion

fixedCount is declared as private readonly ushort (line 33) — a ushort is always ≥ 0, making this.fixedCount >= 0 unconditionally true after widening to int. The check has minor documentation value (signals the assumption that feeds the subsequent subtraction) and zero runtime cost, so this is not a blocker — just noting it for awareness.

💡 Follow-up — Similar unchecked patterns in the same directory

For consistency, these sibling expressions could benefit from checked() in a follow-up:

• DataCollector.AddNullTerminatedString line 159: int size = (nullCharIndex + 1) * 2;
• TraceLoggingDataCollector.AddBinary line 89: value.Length * 2

Both are equally low-probability overflow risks (would require strings approaching int.MaxValue / 2 characters), but the same defense-in-depth argument applies. Not a blocker for this PR.

Generated by Code Review for issue #125759 · ◷