-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix ArgumentOutOfRangeException when Concat called after Skip that skips more elements than exist #123295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ips more elements than exist Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
…ceSpan.Length Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
…perators Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
… operators Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-system-linq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a regression introduced in .NET 10 where Enumerable.Skip() that skips more elements than exist in the source causes an ArgumentOutOfRangeException when followed by certain operators like Concat(), Append(), or when calling ToArray()/ToList().
Changes:
- Added bounds check in
Fill()method to prevent invalid span slicing when skip index exceeds source length - Added comprehensive regression test covering multiple source types and LINQ operators
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/libraries/System.Linq/src/System/Linq/SkipTake.SpeedOpt.cs |
Added bounds check sourceIndex < sourceSpan.Length before calling Slice() to prevent ArgumentOutOfRangeException |
src/libraries/System.Linq/tests/SkipTests.cs |
Added comprehensive test that validates the fix across various source types (array, list, ReadOnlyCollection, etc.) and operators |
eiriktsarpalis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot can you tell if or when this regressed?
|
@copilot can you answer my earlier question please? |
Yes, this regressed in PR #112401 ("Improve Enumerable.Skip and Enumerable.Take performance"), merged on February 13, 2025. That PR added the |
|
@copilot judging by the diff in the PR that you cited, we're missing a similar fix in the IndexOf method? Could you try to address that and add relevant testing? |
Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
The |
|
@stephentoub Any plans for including in 10.0.3 ? |
|
In 10.0.3, definitely not. In a future servicing after that, possibly. |
Enumerable.Concat()throwsArgumentOutOfRangeExceptionwhen called afterSkip()that skips more elements than existIListSkipTakeIterator.Fill()method inSkipTake.SpeedOpt.csSummary
Root Cause:
The
IListSkipTakeIterator.Fill()method inSkipTake.SpeedOpt.cscalledSpan.Slice(sourceIndex, destination.Length)unconditionally. WhenSkip()skips more elements than exist,sourceIndexcan be beyond the source span's length, causingSlice()to throw even though no elements need to be copied.Regression: Introduced in PR #112401 ("Improve Enumerable.Skip and Enumerable.Take performance"), merged on February 13, 2025.
Fix:
Added bounds check
sourceIndex < sourceSpan.Lengthinside the span path before callingSlice().Note on IndexOf: The
IndexOfmethod also added in PR #112401 already has a similar bounds check (minInclusive < span.Length) in the current codebase, so it's not affected by this bug.Test:
Added comprehensive test in
SkipTests.cscovering multiple source types (array, list, ReadOnlyCollection, ICollection, IReadOnlyCollection, enumerable) with various operators:Security Summary:
No security vulnerabilities identified. The fix is a simple bounds check addition that prevents an unnecessary operation.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.