Skip to content

Fix NRange.GetOffsetAndLength truncation: cast to nuint instead of uint#124300

Open
Copilot wants to merge 3 commits intomainfrom
copilot/fix-nrange-getoffsetandlength-bug
Open

Fix NRange.GetOffsetAndLength truncation: cast to nuint instead of uint#124300
Copilot wants to merge 3 commits intomainfrom
copilot/fix-nrange-getoffsetandlength-bug

Conversation

Copy link
Contributor

Copilot AI commented Feb 11, 2026

Description

NRange.GetOffsetAndLength casts its nint parameters to uint for range validation. On 64-bit platforms, this truncates values larger than uint.MaxValue, allowing invalid ranges to pass validation silently.

// Before: truncates upper 32 bits on 64-bit platforms
if ((uint)end > (uint)length || (uint)start > (uint)end)

// After: correct comparison for all nint values
if ((nuint)end > (nuint)length || (nuint)start > (nuint)end)

For example, start = 0x1_0000_0001 and end = 2 with length = 0x1_0000_0003: the (uint) cast truncates start to 1, making 1 > 2 false and incorrectly passing validation.

Note: Other (uint) conversion issues in NRange (e.g., ToString) are intentionally not addressed here — those are covered by #124294.

Tests

  • Converted existing GetOffsetAndLengthTest [Fact] to [Theory] with [InlineData]
  • Split success and exception cases into separate theory methods
  • Added [ConditionalTheory(Is64BitProcess)] tests exercising values above uint.MaxValue, including a case that directly catches the truncation bug

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 11, 2026
… tests

Co-authored-by: vcsjones <361677+vcsjones@users.noreply.github.com>
@vcsjones vcsjones added area-System.Numerics.Tensors and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Feb 11, 2026
Co-authored-by: vcsjones <361677+vcsjones@users.noreply.github.com>
@dotnet-policy-service
Copy link
Contributor

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

Copilot AI changed the title [WIP] Fix bug in NRange.GetOffsetAndLength parameter conversion Fix NRange.GetOffsetAndLength truncation: cast to nuint instead of uint Feb 11, 2026
Copilot AI requested a review from vcsjones February 11, 2026 22:07
@vcsjones vcsjones marked this pull request as ready for review February 11, 2026 22:09
Copilot AI review requested due to automatic review settings February 11, 2026 22:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes incorrect range validation in System.Buffers.NRange.GetOffsetAndLength on 64-bit platforms by avoiding truncating casts, ensuring out-of-range values above uint.MaxValue are rejected correctly.

Changes:

  • Update GetOffsetAndLength validation logic to cast to nuint instead of uint to prevent 64-bit truncation.
  • Refactor GetOffsetAndLength tests from a single [Fact] into multiple [Theory] cases.
  • Add 64-bit-only test cases covering values above uint.MaxValue, including regression coverage for the truncation scenario.

Reviewed changes

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

File Description
src/libraries/System.Numerics.Tensors/src/System/Buffers/NRange.cs Fixes range validation by using nuint casts to avoid 64-bit truncation.
src/libraries/System.Numerics.Tensors/tests/NRangeTests.cs Expands/organizes test coverage and adds 64-bit regression cases for large values.

@vcsjones
Copy link
Member

@copilot please invoke the code-review skill and post your analysis/results as a comment on this PR.

Please keep your review focused just to GetOffsetAndLength. Other related fixes are in #124294 and are out of scope for this review.

Copy link
Contributor Author

Copilot AI commented Feb 12, 2026

@copilot please invoke the code-review skill and post your analysis/results as a comment on this PR.

Please keep your review focused just to GetOffsetAndLength. Other related fixes are in https://g...

🤖 Copilot Code Review (focused on GetOffsetAndLength)

Holistic Assessment

Motivation: The bug is real. NRange.GetOffsetAndLength uses (uint) casts on nint values for bounds validation. On 64-bit, nint is 8 bytes but uint is 4, so values > uint.MaxValue get silently truncated, allowing invalid ranges to pass. This is a correctness bug — the analogous Range.GetOffsetAndLength uses (uint) on int which is same-width and correct.

Approach: The fix is correct and minimal — changing (uint) to (nuint) makes it a same-width unsigned cast, exactly mirroring Range's pattern at the native-integer level.

Summary: ✅ LGTM. The 1-line production fix is correct. The tests are well-structured: the original [Fact] is properly converted to [Theory]/[InlineData], and 64-bit regression tests use [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))] consistent with repo conventions. The scope is appropriately limited to GetOffsetAndLength only.


Detailed Findings

✅ Production fix — Correct (uint)(nuint) cast

The change on line 113 from (uint) to (nuint) is the right fix. Range.GetOffsetAndLength uses (uint) on int parameters (same 4-byte width), so NRange.GetOffsetAndLength should use (nuint) on nint parameters (same native width). This preserves the idiomatic unsigned bounds-check pattern the JIT recognizes while fixing the 64-bit truncation.

✅ Scope — Correctly limited to GetOffsetAndLength

The ToString() method still has (uint) casts on lines 73 and 84, which is intentional — those are addressed by PR #124294. This PR stays focused.

✅ Test structure — Follows repo conventions

  • [Theory] + [InlineData] over individual [Fact] methods ✓
  • [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))] for 64-bit-only tests ✓
  • Using long parameters in [InlineData] with (nint) casts (since nint can't be an attribute constant) ✓
  • Separate theories for success vs. exception cases ✓

✅ Test coverage — Regression case catches the bug

The GetOffsetAndLengthThrowsTest64 case [InlineData((long)uint.MaxValue + 1, false, 2, false, (long)uint.MaxValue + 3)] is a good regression case: start = 0x1_0000_0001 and end = 2 means start > end, which should throw. With the old (uint) cast, start truncates to 1, making 1 > 2 false, so validation incorrectly passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants