JIT: (bug) Signed compare against a path-dependent checked bound eliminates a bounds check
- Dominant language
- C#
- Stars
- 18.3k
- Forks
- 5.6k
- PR merge metrics
- PR metrics pending
Description
RangeCheck treats `new int[n]`'s length as a method-wide "checked bound" for the VN of `n` once any block indexes that array. On a path where the allocation never happened and `n` is negative, a signed `k >= n` compare is still consumed as a bound-relative fact, so the index is "proven" non-negative and the bounds check for `arr[i]` is removed.
### Minimal Repro
```csharp
using System;
using System.Runtime.CompilerServices;
public class Program
{
static int[] s_arr = new int[8];
static int s_sink;
[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
static int Test(int[] arr, int n, int k, bool cond)
{
if (cond)
{
int[] tmp = new int[n];
s_sink = tmp[0];
}
int i = 0;
if (k >= n)
{
i = k;
}
if (i < arr.Length)
{
return arr[i];
}
return -1;
}
static int Main()
{
for (int j = 0; j < 3; j++)
{
Test(s_arr, 4, 2, true);
}
try
{
Console.WriteLine("No exception, returned " + Test(s_arr, -5, -3, false));
}
catch (IndexOutOfRangeException)
{
Console.WriteLine("IndexOutOfRangeException");
}
return 100;
}
}
```
In the final call `cond == false`, `k >= n` is `-3 >= -5`, so `i == -3` and `-3 < 8` holds — `arr[-3]` must throw.
### Expected
```
IndexOutOfRangeException
```
### Actual
```
UB
```
The codegen emits `mov eax, eax` (zero-extend of a negative index) and no `CORINFO_HELP_RNGCHKFAIL` for the load; the only one left belongs to `tmp[0]`. `DOTNET_JITMinOpts=1` is correct.
### Notes
- x64, `main` @ `6ed2ba318dcaa936da60f7d6bc9eb30cad54fc0c`; also reproduces on .NET 10.0.12 Release, so it affects servicing branches (not bisected).
- `ARR_LENGTH(new int[n])` folds to the VN of `n`, so `tmp[0]` registers `n` method-wide as a checked bound `$bnd`.
- On the `cond == false` path the signed `k >= n` yields a `keBinOpArray($n, 0)` lower limit for `i`, and `Range::Merge`'s `<$bnd + cns1, ...> U ` rule collapses it to the constant `0`, making the upper-bound-only `i < arr.Length` check look sufficient.
- Silent memory-safety hole: with a suitable length/index this is an arbitrary in-process OOB read rather than a crash.
Contributor guide
Research direction
Reproduce the minimal C# program on the stated runtime commit, then trace the JIT RangeCheck and Range::Merge handling of ARR_LENGTH, keBinOpArray, and the checked bound across the cond == false path. Done means the bounds check is retained and the final call produces the expected IndexOutOfRangeException, with a regression test covering the path-dependent case.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100