JIT: loop cloning generates unsafe fast clone for decreasing loops when limit and indexed arrays differ

[AndyAyersMS]

JIT: loop cloning generates unsafe fast clone for decreasing loops when limit and indexed arrays differ

Note

This issue was investigated and drafted with GitHub Copilot CLI.

Description

optDeriveLoopCloningConditions derives a limit <relop> arr.Length condition for the per-access bounds-check elision, where limit is the loop test's RHS. For increasing loops this is sound because the visited indices are [init, limit) and init is independently constrained >= 0. For decreasing loops the visited indices are [limit+1, init], so the value that must fit in the indexed array is init, not limit. The current code only constrains init against the array length when the limit is a constant or an invariant local — when HasArrayLengthLimit is true, ident is set to the limit (loopcloning.cpp ~line 1419) and no bound on init is emitted, so the fast clone (with bounds checks removed) is taken even when init is far outside the indexed array.

This is benign in the very common shape for (i = a.Length - 1; i > 0; i--) a[i]… because there init and the indexed array agree and the array-deref/length condition implicitly bounds init. It becomes unsafe as soon as the loop test's limit array differs from the array being indexed inside the body.

Reproduction

Repository: dotnet/runtime
Tested on: main at 9593745 (also reproduces on .NET 10 RTM)
Configuration: windows.x64.Checked, FullOpts, tiering disabled.

using System;
using System.Runtime.CompilerServices;

public class P
{
 [MethodImpl(MethodImplOptions.NoInlining)]
 public static int LatentBugGT(byte[] limitArr, byte[] accessArr, int startIdx)
 {
 int sum = 0;
 for (int i = startIdx; i > limitArr.Length; i--)
 sum += accessArr[i];
 return sum;
 }

 [MethodImpl(MethodImplOptions.NoInlining)]
 public static int LatentBugGE(byte[] limitArr, byte[] accessArr, int startIdx)
 {
 int sum = 0;
 for (int i = startIdx; i >= limitArr.Length; i--)
 sum += accessArr[i];
 return sum;
 }

 public static int Main()
 {
 Console.WriteLine("=== GT ===");
 try { int x = LatentBugGT(new byte[1], new byte[5], 1000); Console.WriteLine($"BUG: returned {x}"); }
 catch (IndexOutOfRangeException) { Console.WriteLine("OK: threw IndexOutOfRangeException"); }

 Console.WriteLine("=== GE ===");
 try { int x = LatentBugGE(new byte[1], new byte[5], 1000); Console.WriteLine($"BUG: returned {x}"); }
 catch (IndexOutOfRangeException) { Console.WriteLine("OK: threw IndexOutOfRangeException"); }

 return 100;
 }
}

Expected

Both calls index accessArr[1000] (length 5) and must throw IndexOutOfRangeException.

Actual

=== GT ===
BUG: returned 0
=== GE ===
BUG: returned 0

Both loops silently iterate i = 1000, 999, …, 2 (or …, 1 for GE) and dereference accessArr[i] for ~999 out-of-bounds indices with no exception. Whatever the GC heap happens to contain at those offsets is summed and returned. (In the small repro above the sum happens to be 0; with arbitrary heap state this would silently corrupt analysis results, or — for write loops — silently corrupt heap memory.)

Cloning conditions emitted

JIT dump from the unmodified clrjit.dll:

Conditions: (V04 GE 0) && (V00.Length LT V01.Length)
Considering condition 0: (V04 GE 0), could not be evaluated
Considering condition 1: (V00.Length LT V01.Length), could not be evaluated
Loops cloned: 1
Loops statically optimized: 0

V04 = startIdx, V00 = limitArr, V01 = accessArr. With the bad inputs startIdx=1000, limitArr.Length=1, accessArr.Length=5:

condition	actual
startIdx >= 0	1000 >= 0 ✓
limitArr.Length < accessArr.Length	1 < 5 ✓

Both pass → fast clone runs without bounds checks → OOB reads of accessArr.

Root cause

In src/coreclr/jit/loopcloning.cpp optDeriveLoopCloningConditions:

• For HasConstInit decreasing loops, ident = init (line ~1348).
• For variable-init decreasing loops, ident = initVar (line ~1370).
• For HasArrayLengthLimit, ident is unconditionally set to the limit array's length (~line 1419), regardless of direction. The decreasing path then emits cond(opLimitCondition, ident, accessArr.Length) which evaluates to limitArr.Length < accessArr.Length — a constraint on limit, not init.

The cloning condition needed for soundness in the decreasing direction is init < accessArr.Length (for GT) or init <= accessArr.Length (for GE), independent of what the limit array is.

Suggested fix

For decreasing loops with HasArrayLengthLimit, do not let the limit array length be used as ident. Instead, always materialize an independent init-based identifier (from iterInfo->IterVar at the preheader, or iterInfo->ConstInitValue) and emit the per-access init <relop> accessArr.Length condition against it. The increasing-loop path can be left as is.

Why this isn't fixed in PR for #84697

This was found while reviewing the changes for #84697 (bounds-check elimination for i != arr.Length loops). To keep that PR's blast radius bounded I restricted GT_NE support to increasing loops only, with an explicit comment noting that decreasing GT_NE would have inherited (and widened) this bug into the very common for (i = end; i != start; i--) idiom. The regression test in that PR (Runtime_84697.SumDecreasingNETwoArrays) pins the safe behavior for GT_NE. The pre-existing GT_GT/GT_GE versions documented here remain.

Suggested area / labels

• area-CodeGen-coreclr
• tenet-correctness
• bug

category

correctness

theme

loop-cloning · bounds-checks

skill-level

expert

cost

medium

impact

medium (correctness — silent OOB read in an uncommon but legitimate code shape)

[AndyAyersMS]

Note

AI-assisted analysis (GitHub Copilot CLI).

Bug provenance

The bug was introduced in #67930 (commit 1bc09e76f04, Apr 26 2022) which added the isIncreasingLoop/isDecreasingLoop distinction to optDeriveLoopCloningConditions. That PR correctly gated the LPFLG_CONST_LIMIT and LPFLG_VAR_LIMIT branches on isIncreasingLoop (so they only assign ident = limit for increasing loops), but the parallel LPFLG_ARRLEN_LIMIT branch was missed — it kept the unconditional ident = LC_Ident(LC_Array(...)) that was correct only under the original "increasing only" assumption.

The misconception is visible in the comment block that was added in the same PR:

// Increasing loops
// GT_LT loop test: (start < end) ==> (end <= arrLen)
// GT_LE loop test: (start <= end) ==> (end < arrLen)
//
// Decreasing loops
// GT_GT loop test: (end > start) ==> (end <= arrLen)
// GT_GE loop test: (end >= start) ==> (end < arrLen)

For decreasing loops the upper end of the IV's range is init (i.e. the LHS of the test), not end/limit; the conditions should be init <= arrLen / init < arrLen. The current per-access condition ident <relop> accessArr.Length is therefore correct only when ident represents init for decreasing loops — which the other two limit branches achieve by leaving ident alone, but the ARRLEN_LIMIT branch silently overwrote.

Release impact

1bc09e76f04 is in v7.0.0-preview.5.22301.12, so the bug shipped in:

• .NET 7 (RTM Nov 2022)
• .NET 8
• .NET 9
• .NET 10 (current main)

In other words, ~4 years and 4 major releases. The reason it appears to have gone unreported is that the unsafe shape requires:

• A decreasing loop
• Whose loop test limit is an array length
• Where the indexed array inside the body is a different object than the array supplying the limit
• And the init value is allowed to be >= accessArr.Length

The much more common decreasing-loop idiom for (i = a.Length - 1; i >= 0; i--) a[i] uses LPFLG_CONST_LIMIT (limit 0), not LPFLG_ARRLEN_LIMIT, so it is unaffected.