Arm64: If Conversion can bypass overflow checks

[a74nh]

This issue was spotted as part of PR #77728

    [MethodImplAttribute(MethodImplOptions.NoInlining)]
    static void lenfail(uint[] a, uint i)
    {
        if (i < a.Length)
	 {
           i = a[i];
	 }
	Consume(i,i+2);
    }

    static void Main(string[] args)
    {
	 var arr = new uint[] { 1, 42, 3000 };
	 lenfail(arr,0xffffffff);
    }

With the following set:

export DOTNET_TieredCompilation=0
export DOTNET_JitStressModeNames=STRESS_IF_CONVERSION_COST
export DOTNET_JITMinOpts=0

Get error:

Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at CSharpTutorials.Program.lenfail(UInt32[], UInt32)
   at CSharpTutorials.Program.Main(System.String[])
[1]    2099041 abort (core dumped)   ./bin/release/net7.0/linux-arm64/conditional.dll

Why? The load from a[i] should only happen if the if condition holds. With If Conversion, the load is effectively hoisted outside if and is always executed.

This is the Assignment block the If Conversion sees. There are no flags here to indicate moving the ASG before the compare is unsafe. Even more worrying, the IND is marked as GTF_IND_NONFAULTING.

STMT00003 ( 0x008[E-] ... 0x00B )
N012 ( 10, 12) [000018] -A--G---R--                         *  ASG       int    $145
N011 (  1,  1) [000017] D------N---                         +--*  LCL_VAR   int    V01 arg1         d:3 $VN.Void
N010 ( 10, 12) [000031] n---G--N---                         \--*  IND       int    <l:$183, c:$c2>
N009 (  7, 10) [000029] -----------                            \--*  ARR_ADDR  byref int[] $2c0
N008 (  7, 10) [000028] -------N---                               \--*  ADD       byref  $201
N003 (  3,  4) [000027] -----------                                  +--*  ADD       byref  $200
N001 (  1,  1) [000019] -----------                                  |  +--*  LCL_VAR   ref    V00 arg0         u:1 (last use) $80
N002 (  1,  2) [000026] -----------                                  |  \--*  CNS_INT   long   16 $1c0
N007 (  4,  6) [000025] -----------                                  \--*  LSH       long   $241
N005 (  2,  3) [000023] ---------U-                                     +--*  CAST      long <- uint $240
N004 (  1,  1) [000020] -----------                                     |  \--*  LCL_VAR   int    V01 arg1         u:1 (last use) $c0
N006 (  1,  2) [000024] -------N---                                     \--*  CNS_INT   long   2 $1c1

Not quite sure yet on what needs to be fixed earlier on

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This issue was spotted as part of PR #77728

    [MethodImplAttribute(MethodImplOptions.NoInlining)]
    static void lenfail(uint[] a, uint i)
    {
        if (i < a.Length)
	 {
           i = a[i];
	 }
	Consume(i,i+2);
    }

    static void Main(string[] args)
    {
	 var arr = new uint[] { 1, 42, 3000 };
	 lenfail(arr,0xffffffff);
    }

With the following set:

export DOTNET_TieredCompilation=0
export DOTNET_JitStressModeNames=STRESS_IF_CONVERSION_COST
export DOTNET_JITMinOpts=0

Get error:

Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at CSharpTutorials.Program.lenfail(UInt32[], UInt32)
   at CSharpTutorials.Program.Main(System.String[])
[1]    2099041 abort (core dumped)   ./bin/release/net7.0/linux-arm64/conditional.dll

Why? The load from a[i] should only happen if the if condition holds. With If Conversion, the load is effectively hoisted outside if and is always executed.

This is the Assignment block the If Conversion sees. There are no flags here to indicate moving the ASG before the compare is unsafe. Even more worrying, the IND is marked as GTF_IND_NONFAULTING.

STMT00003 ( 0x008[E-] ... 0x00B )
N012 ( 10, 12) [000018] -A--G---R--                         *  ASG       int    $145
N011 (  1,  1) [000017] D------N---                         +--*  LCL_VAR   int    V01 arg1         d:3 $VN.Void
N010 ( 10, 12) [000031] n---G--N---                         \--*  IND       int    <l:$183, c:$c2>
N009 (  7, 10) [000029] -----------                            \--*  ARR_ADDR  byref int[] $2c0
N008 (  7, 10) [000028] -------N---                               \--*  ADD       byref  $201
N003 (  3,  4) [000027] -----------                                  +--*  ADD       byref  $200
N001 (  1,  1) [000019] -----------                                  |  +--*  LCL_VAR   ref    V00 arg0         u:1 (last use) $80
N002 (  1,  2) [000026] -----------                                  |  \--*  CNS_INT   long   16 $1c0
N007 (  4,  6) [000025] -----------                                  \--*  LSH       long   $241
N005 (  2,  3) [000023] ---------U-                                     +--*  CAST      long <- uint $240
N004 (  1,  1) [000020] -----------                                     |  \--*  LCL_VAR   int    V01 arg1         u:1 (last use) $c0
N006 (  1,  2) [000024] -------N---                                     \--*  CNS_INT   long   2 $1c1

Not quite sure yet on what needs to be fixed earlier on

Author:	a74nh
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[jakobbotsch]

Even more worrying, the IND is marked as GTF_IND_NONFAULTING.

This part is expected. It is how we represent these kinds of indirs.
The missing bit here is really the GTF_ORDER_SIDEEFF that indicates that reordering the node can introduce new side effects.

[a74nh]

The missing bit here is really the GTF_ORDER_SIDEEFF

Agreed.

Not sure how relevant this part is, but this feels wrong:

During fgMorphBlocks, a range check is added to the IND.

Much later optRemoveRangeCheck decides that the bounds check can be replaced with a NOP - the pass only checks if the BOUNDS_CHECK has GTF_ASG set.

Afterwards there is nothing left with GTF_EXCEPT set

Before optRemoveRangeCheck:
N014 ( 18, 22) [000030] ---XG------                         *  COMMA     int    <l:$184, c:$185>
N003 (  8, 10) [000022] ---X-------                         +--*  BOUNDS_CHECK_Rng void   $145
N001 (  1,  1) [000014] -----------                         |  +--*  LCL_VAR   int    V01 arg1         u:1 $c0
N002 (  3,  2) [000041] -----------                         |  \--*  LCL_VAR   int    V03 cse0         u:1 $c1
N013 ( 10, 12) [000031] n---G------                         \--*  IND       int    <l:$183, c:$c2>
N012 (  7, 10) [000029] -----------                            \--*  ARR_ADDR  byref int[] $2c0
N011 (  7, 10) [000028] -------N---                               \--*  ADD       byref  $201
N006 (  3,  4) [000027] -----------                                  +--*  ADD       byref  $200
N004 (  1,  1) [000019] -----------                                  |  +--*  LCL_VAR   ref    V00 arg0         u:1 (last use) $80
N005 (  1,  2) [000026] -----------                                  |  \--*  CNS_INT   long   16 $1c0
N010 (  4,  6) [000025] -----------                                  \--*  LSH       long   $241
N008 (  2,  3) [000023] ---------U-                                     +--*  CAST      long <- uint $240
N007 (  1,  1) [000020] -----------                                     |  \--*  LCL_VAR   int    V01 arg1         u:1 (last use) $c0
N009 (  1,  2) [000024] -------N---                                     \--*  CNS_INT   long   2 $1c1

After optRemoveRangeCheck for [000030]:
N016 ( 18, 22) [000018] -A--G---R--                         *  ASG       int    $145
N015 (  1,  1) [000017] D------N---                         +--*  LCL_VAR   int    V01 arg1         d:3 $VN.Void
N014 ( 18, 22) [000030] ----G--N---                         \--*  COMMA     int    <l:$184, c:$185>
N003 (  8, 10) [000022] -----------                            +--*  NOP       void  
N013 ( 10, 12) [000031] n---G------                            \--*  IND       int    <l:$183, c:$c2>
N012 (  7, 10) [000029] -----------                               \--*  ARR_ADDR  byref int[] $2c0
N011 (  7, 10) [000028] -------N---                                  \--*  ADD       byref  $201
N006 (  3,  4) [000027] -----------                                     +--*  ADD       byref  $200
N004 (  1,  1) [000019] -----------                                     |  +--*  LCL_VAR   ref    V00 arg0         u:1 (last use) $80
N005 (  1,  2) [000026] -----------                                     |  \--*  CNS_INT   long   16 $1c0
N010 (  4,  6) [000025] -----------                                     \--*  LSH       long   $241
N008 (  2,  3) [000023] ---------U-                                        +--*  CAST      long <- uint $240
N007 (  1,  1) [000020] -----------                                        |  \--*  LCL_VAR   int    V01 arg1         u:1 (last use) $c0
N009 (  1,  2) [000024] -------N---                                        \--*  CNS_INT   long   2 $1c1

[a74nh]

Not sure how relevant this part is, but this feels wrong:

Oh - Assertion prop has already decided that it's safe to remove the bounds check due to the earlier compare.

Which probably means the fix here is that if optRemoveRangeCheck() removes a BOUNDS_CHECK_Rng then it (or it's parent function) should be setting GTF_ORDER_SIDEEFF on the IND

[jakobbotsch]

Which probably means the fix here is that if optRemoveRangeCheck() removes a BOUNDS_CHECK_Rng then it (or it's parent function) should be setting GTF_ORDER_SIDEEFF on the IND

That sounds right to me.