JIT: guard overflow in loop cloning offset limits - #133834
Conversation
Positive array length offsets can wrap negative and let the fast clone run without bounds checks. Guard offsets that can overflow and add regression coverage for decreasing loops. Fixes dotnet#133823 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 336fb408-8580-4f7b-b3e6-087280a6294a
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review issues were identified.
Pull request overview
Guards JIT loop cloning against signed overflow in positive array-length offsets and adds regression coverage.
Changes:
- Adds overflow detection for positive offset limits.
- Tests decreasing loops using
>,>=, and!=.
File summaries
| File | Description |
|---|---|
src/tests/JIT/opt/Cloning/OffsetLimit.cs |
Adds positive-offset overflow regression tests. |
src/coreclr/jit/loopcloning.cpp |
Adds an overflow-aware fast-clone guard. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
|
@dhartglassMSFT ptal Smallish fix in cloning checks. |
|
fyi one of the new tests fails on mono linux x64, I don't know whether we're keeping up with these, change LGTM otherwise: |
I'll add an exclusion and ping you for reapproval. |
Mono LLVM AOT crashes instead of throwing for the GT overflow case. Split it from the passing theory rows and track the failure in dotnet#134014. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 336fb408-8580-4f7b-b3e6-087280a6294a
|
@dhartglassMSFT re-approve when you get a chance. |
The first exclusion worked, but CI then hit the same crash in the GE case. Exclude all equivalent overflow rows under dotnet#134014. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 336fb408-8580-4f7b-b3e6-087280a6294a
|
@dhartglassMSFT we needed more mono exclusions. Hopefully got them all this time. |
Positive array length offsets can wrap negative and let the fast clone run without bounds checks. Guard offsets that can overflow and add regression coverage for decreasing loops. Fixes dotnet#133823 > [!NOTE] > This pull request description was generated with GitHub Copilot. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 336fb408-8580-4f7b-b3e6-087280a6294a
|
/backport to release/11.0 |
|
Started backporting to |
…4350) Backport of #133834 to release/11.0 /cc @AndyAyersMS ## Customer Impact - [ ] Customer reported - [x] Found internally Loop cloning can incorrectly remove array bounds checks when a positive `arr.Length + offset` loop limit overflows to a negative value. Safe C# can then perform an out-of-bounds read and terminate with a native access violation instead of throwing `IndexOutOfRangeException`. This reproduces on .NET 11 RC1 and RC2; .NET 10 is correct. See #133823. ## Regression - [x] Yes - [ ] No Introduced during the .NET 11 cycle by #129309, which extended loop cloning to handle span/stride and constant-offset limits. ## Testing Added regression coverage for decreasing loops with overflowing positive offsets. The test fails before the fix and verifies that the required bounds exception is preserved after the fix. The original PR also passed JIT CI; Mono exclusions were added because this is CoreCLR-specific loop-cloning coverage. ## Risk Low. The change is localized to loop-cloning condition derivation. It rejects offsets that cannot be represented safely instead of selecting the unchecked fast clone, so the fallback retains the existing bounds checks. Co-authored-by: Andy Ayers <andya@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 336fb408-8580-4f7b-b3e6-087280a6294a
Positive array length offsets can wrap negative and let the fast clone run
without bounds checks. Guard offsets that can overflow and add regression
coverage for decreasing loops.
Fixes #133823
Note
This pull request description was generated with GitHub Copilot.