dotnet / dotnet/runtime

JIT: (bug) Signed compare against a path-dependent checked bound eliminates a bounds check

Open
#133,821 1 comment 0 reactions 0 assignees View on GitHub
area-CodeGen-coreclr
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.