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 and Claude Sonnet 4.6.

Holistic Assessment

Motivation: Justified. Integer overflow in buffer-size computations is a well-documented class of memory-safety bugs. The JSHostImplementation.GetMethodSignature change is the most impactful — the old unchecked arithmetic could silently wrap size to a small value with a pathologically long functionName/moduleName, causing NativeMemory.Alloc to under-allocate followed by out-of-bounds writes. The other changes are lower-stakes but consistent defensive hardening.

Approach: Correct. checked() is the idiomatic .NET tool for guarding arithmetic that feeds allocation sizes. Debug.Assert is appropriate for internal invariant validation in private/internal methods where callers already maintain the invariant and Buffer.MemoryCopy provides release-build protection.

Summary: ✅ LGTM. All four changes are correct, consistent with existing patterns in their respective files, and introduce no behavioral change for valid inputs. Two minor observations below.

---

Detailed Findings

✅ Security — JSHostImplementation.GetMethodSignature overflow fix is correct and meaningful

The old code computed the native allocation size with three unchecked arithmetic expressions. With a string of ~1 billion characters, functionName.Length * 2 wraps negative, causing NativeMemory.Alloc to allocate a tiny buffer. The subsequent Unsafe.CopyBlock writes would corrupt adjacent memory. The new checked() expressions correctly cover all three risk points: the header+type product, the function/module name byte counts, and the cumulative size additions. The restructuring from three separate size += statements into consolidated checked() expressions also improves readability.

✅ Correctness — DataCollector.AddArray checked multiplication is consistent

length is capped to ushort.MaxValue (65535) and itemSize is a compile-time sizeof(T) (typically 1–16), so the product can't realistically overflow (~1 MB max). 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. Zero risk, maximum consistency.

✅ Safety — SecureString.UnmanagedBuffer.Copy assertion is adequate

Buffer.MemoryCopy already validates bytesLength > destinationSizeInBytes at runtime in Release builds (throws ArgumentOutOfRangeException), so the Debug.Assert serves as a development-time early indicator, not the sole line of defense. Both callers maintain the invariant through normal control flow — the copy constructor uses str._buffer.ByteLength for both allocation and copy, and EnsureCapacity allocates a larger buffer before copying.

💡 Suggestion — FieldMetadata assertion has a tautological sub-condition

In FieldMetadata.Encode, the assertion:

Debug.Assert(pos >= 0 && this.fixedCount >= 0 && pos <= metadata.Length - this.fixedCount);

fixedCount is declared as private readonly ushort (line 33) — a ushort is always ≥ 0, making this.fixedCount >= 0 unconditionally true. The check has documentation value (signals intent that the math assumes non-negative values) and zero runtime cost, so this is not a blocker. The destination bounds check (pos <= metadata.Length - this.fixedCount) is correct and safe: since fixedCount maxes at 65535 and metadata.Length is a non-negative int, the subtraction cannot underflow to a misleading value.

💡 Suggestion — New OverflowException path in GetMethodSignature is untested (follow-up)

The PR changes the failure mode from silent memory corruption to OverflowException for pathologically long strings. This is a clear security win, but the new throwing path isn't exercised by any test. In practice, testing this requires allocating a >1 GB string, which is impractical for CI. If a lightweight test is desired, it could be deferred to a follow-up. Not a merge blocker.

---

Models contributing: Claude Opus 4.6 (primary), Claude Sonnet 4.6. GPT-5.4 agent timed out and did not contribute.

Generated by Code Review for issue #125759 · ◷