Fix integer overflow in ArrayList.IListWrapper.BinarySearch and improve test coverage

[Copilot]

Description

The IListWrapper.BinarySearch implementation computed the midpoint as mid = (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

• Replace overflow-prone midpoint calculation with the same approach used in Array.GetMedian: mid = lo + ((hi - lo) >> 1)
• Move mid declaration inside the loop
• Add test BinarySearch_LargeList_NoIntegerOverflow that validates the fix using a simulated large IList to avoid memory allocation issues in CI
• Add 7 comprehensive tests to improve BinarySearch 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
• Add ThrowingComparer helper class for exception testing
• Add FakeLargeIList helper class that simulates an array of int.MaxValue / 2 + 2 elements without actually allocating memory, making it suitable for CI environments

This matches the pattern already used in Array.BinarySearch and prevents overflow by ensuring (hi - lo) remains in bounds before the division.

// Before (overflows)
int mid = (lo + hi) / 2;

// After (overflow-safe)
int mid = lo + ((hi - lo) >> 1);

Additional Investigation

Investigated other types in the repository for similar overflow risks. Found that:

• Array.cs and ArraySortHelper.cs already use the safe pattern
• ExpressionParser.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 risk

Testing

• ✅ All 20 BinarySearch tests pass (12 original + 8 new)
• ✅ Comprehensive coverage of all 3 method overloads
• ✅ Edge cases covered: empty lists, single/two element lists
• ✅ Boundary conditions and partial range searches tested
• ✅ Exception handling verified
• ✅ Code review completed with no issues
• ✅ Security scan completed

---

💡 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.

[dotnet-policy-service]

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

[Copilot]

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) / 2 with mid = lo + ((hi - lo) >> 1) to prevent integer overflow when searching large arrays
• Move the mid variable declaration inside the while loop to limit its scope

[stephentoub]

@copilot, do any other types in this repo have a similar overflow risk?

[Copilot]

Found 3 additional locations with similar overflow risk using (lo + hi) / 2:

1. System.Data.Common - ExpressionParser.cs:1124 (binary search in reserved words array)

   • Low risk: Fixed-size array of ~12 reserved words

2. System.Private.CoreLib - ConcurrentSet.cs:36 and ConcurrentSet.cs:81 (binary search in TraceLogging)

   • Medium risk: Array grows dynamically but overflow extremely unlikely in practice

All other binary search implementations in System.Private.CoreLib (Array.cs, ArraySortHelper.cs, List via ArraySortHelper) already use the safe pattern lo + ((hi - lo) >> 1).

Should I file separate issues for the ConcurrentSet instances, or would you prefer I fix them in this PR?

[stephentoub]

@copilot nit maybe this should be called FakeLargeIList instead

[Copilot]

Renamed to FakeLargeIList in commit 0ecedca.