Fix integer overflow in ArrayList.IListWrapper.BinarySearch and improve test coverage#123806
Merged
stephentoub merged 6 commits intomainfrom Feb 10, 2026
Merged
Fix integer overflow in ArrayList.IListWrapper.BinarySearch and improve test coverage#123806stephentoub merged 6 commits intomainfrom
stephentoub merged 6 commits intomainfrom
Conversation
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-collections |
7 tasks
Co-authored-by: vcsjones <[email protected]>
Copilot
AI
changed the title
[WIP] Fix issue 123804 in runtime
Fix integer overflow in ArrayList.IListWrapper.BinarySearch
Jan 30, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an integer overflow vulnerability in ArrayList.IListWrapper.BinarySearch by replacing the overflow-prone midpoint calculation with the overflow-safe pattern already used throughout the .NET runtime.
Changes:
- Replace
mid = (lo + hi) / 2withmid = lo + ((hi - lo) >> 1)to prevent integer overflow when searching large arrays - Move the
midvariable declaration inside the while loop to limit its scope
This was referenced Jan 30, 2026
stephentoub
reviewed
Jan 31, 2026
src/libraries/System.Private.CoreLib/src/System/Collections/ArrayList.cs
Show resolved
Hide resolved
Add BinarySearch_LargeList_NoIntegerOverflow test that verifies the fix handles large list indices correctly without integer overflow. The test uses a simulated large list to avoid memory allocation issues in CI. Co-authored-by: stephentoub <[email protected]>
stephentoub
reviewed
Jan 31, 2026
src/libraries/System.Private.CoreLib/src/System/Collections/ArrayList.cs
Show resolved
Hide resolved
src/libraries/System.Collections.NonGeneric/tests/ArrayListTests.cs
Outdated
Show resolved
Hide resolved
Add 7 additional tests to improve coverage: - BinarySearch_EmptyList: Test searching in empty list - BinarySearch_SingleElement: Test single element edge case - BinarySearch_BoundaryConditions: Test search at start/end of ranges - BinarySearch_PartialRangeSearch: Test searching within subranges - BinarySearch_ComparerThrowsException: Test exception propagation from comparer - BinarySearch_UnsortedList: Test behavior on unsorted list (should not crash) - BinarySearch_TwoElementList: Test two-element edge case All 20 BinarySearch tests now pass. Co-authored-by: stephentoub <[email protected]>
Co-authored-by: stephentoub <[email protected]>
Copilot
AI
changed the title
Fix integer overflow in ArrayList.IListWrapper.BinarySearch
Fix integer overflow in ArrayList.IListWrapper.BinarySearch and improve test coverage
Jan 31, 2026
Co-authored-by: stephentoub <[email protected]>
stephentoub
previously approved these changes
Feb 2, 2026
stephentoub
approved these changes
Feb 10, 2026
jkotas
approved these changes
Feb 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The
IListWrapper.BinarySearchimplementation computed the midpoint asmid = (lo + hi) / 2, which overflows when searching large arrays (e.g.,byte[int.MaxValue / 2 + 2]). This PR fixes the overflow issue and significantly improves test coverage for the BinarySearch method.Fixes #123804
Changes
Array.GetMedian:mid = lo + ((hi - lo) >> 1)middeclaration inside the loopBinarySearch_LargeList_NoIntegerOverflowthat validates the fix using a simulated large IList to avoid memory allocation issues in CIBinarySearch_EmptyList: Test searching in empty listBinarySearch_SingleElement: Test single element edge caseBinarySearch_BoundaryConditions: Test search at start/end of rangesBinarySearch_PartialRangeSearch: Test searching within subrangesBinarySearch_ComparerThrowsException: Test exception propagation from comparerBinarySearch_UnsortedList: Test behavior on unsorted list (should not crash)BinarySearch_TwoElementList: Test two-element edge caseThrowingComparerhelper class for exception testingFakeLargeIListhelper class that simulates an array ofint.MaxValue / 2 + 2elements without actually allocating memory, making it suitable for CI environmentsThis matches the pattern already used in
Array.BinarySearchand prevents overflow by ensuring(hi - lo)remains in bounds before the division.Additional Investigation
Investigated other types in the repository for similar overflow risks. Found that:
Array.csandArraySortHelper.csalready use the safe patternExpressionParser.cs(System.Data.Common) has the unsafe pattern but very low risk (small fixed array)ConcurrentSet.cs(TraceLogging) has the unsafe pattern with medium theoretical riskTesting
💡 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.