Runtime: RyuJIT: By-ref assignment with null leads to runtime crash

[jakobbotsch]

The following example crashes the runtime when compiled and run in either debug or release:

class C0
{
}

struct S0
{
    public C0 F0;
    public ulong F4;
}

class C1
{
    public S0 F3;
}

struct S1
{
    public S0 F3;
}

public class Program
{
    static S1 s_38;
    static C1 s_43;

    public static void Main()
    {
        s_38.F3 = s_43.F3;
    }
}

In debug (and probably checked) builds, this assertion triggers:

Assert failure(PID 20848 [0x00005170], Thread: 17420 [0x440c]): Consistency check failed: AV in clr at this callstack:
------
CORECLR! JIT_ByRefWriteBarrier + 0x0 (0x00007ffc`5e207d70)
<no module>! <no symbol> + 0x0 (0x00007ffb`fef22611)
<no module>! <no symbol> + 0x0 (0x00007ffb`fee045e0)
<no module>! <no symbol> + 0x0 (0x000000fa`00000005)
<no module>! <no symbol> + 0x0 (0x000000fa`2077d580)
-----
.AV on tid=0x440c (17420), cxr=000000FA2077CE00, exr=000000FA2077D2F0
FAILED: false

CORECLR! CHECK::Trigger + 0x275 (0x00007ffc`5db0cea5)
CORECLR! CLRVectoredExceptionHandlerPhase3 + 0x332 (0x00007ffc`5dc1bd62)
CORECLR! CLRVectoredExceptionHandlerPhase2 + 0x8C (0x00007ffc`5dc1b70c)
CORECLR! CLRVectoredExceptionHandler + 0x275 (0x00007ffc`5dc1b665)
CORECLR! CLRVectoredExceptionHandlerShim + 0x18D (0x00007ffc`5dc1c04d)
NTDLL! RtlInitializeCriticalSection + 0x1D8 (0x00007ffc`c5bd5678)
NTDLL! RtlWalkFrameChain + 0x109A (0x00007ffc`c5b7692a)
NTDLL! KiUserExceptionDispatcher + 0x2E (0x00007ffc`c5c0dc1e)
CORECLR! JIT_ByRefWriteBarrier + 0x0 (0x00007ffc`5e207d70)
<no module>! <no symbol> + 0x0 (0x00007ffb`fef22611)
    File: z:\programming\dotnet\coreclr\src\vm\excep.cpp Line: 7831
    Image: Z:\Programming\dotnet\coreclr\bin\Product\Windows_NT.x64.Debug\CoreRun.exe

The code generated is:

; Assembly listing for method Program:Main()
; Emitting BLENDED_CODE for X64 CPU with AVX
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
;  V00 OutArgs      [V00    ] (  1,  1   )  lclBlk (32) [rsp+0x00]
;  V01 cse0         [V01,T00] (  5,  5   )    long  ->  [rsp+0x20]
;
; Lcl frame size = 40

G_M5092_IG01:
       57                   push     rdi
       56                   push     rsi
       4883EC28             sub      rsp, 40

G_M5092_IG02:
       48B9E045E3FEFB7F0000 mov      rcx, 0x7FFBFEE345E0
       BA05000000           mov      edx, 5
       E8461E455F           call     CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
       48B87829001091010000 mov      rax, 0x19110002978
       488B00               mov      rax, gword ptr [rax]
       488D7008             lea      rsi, bword ptr [rax+8]
       48B87029001091010000 mov      rax, 0x19110002970
       488B00               mov      rax, gword ptr [rax]
       488D7808             lea      rdi, bword ptr [rax+8]
       E85F572B5F           call     CORINFO_HELP_ASSIGN_BYREF
       48A5                 movsq

G_M5092_IG03:
       4883C428             add      rsp, 40
       5E                   pop      rsi
       5F                   pop      rdi
       C3                   ret

; Total bytes of code 74, prolog size 6 for method Program:Main()
;

Question: How will this normally be handled by CoreCLR? Should the runtime turn the AV in CORINFO_HELP_ASSIGN_BYREF into a managed NRE, or is the caller supposed to null-check/deref the reference before calling CORINFO_HELP_ASSIGN_BYREF?

[danmosemsft]

@dotnet/jit-contrib this crashes in desktop also.

[AndyAyersMS]

I'll investigate.

[AndyAyersMS]

The question is whether we are crashing in the right fashion -- whether the jit should be triggering detecting this before we get into the helper, or whether the runtime can cope with an AV in the helper.

My understanding is that an AV in a helper is only ok for helpers described by IsIPInMarkedJitHelper and CORINFO_HELP_ASSIGN_BYREF is not in this set.

So that would seem to imply that the jit must do an explicit null check here before calling the helper.

If we change S0 to be int we get the expected unhandle exception exit.

[jkotas]

helpers described by IsIPInMarkedJitHelper

This set is not in stone. It should ok to add to it if it makes a difference.

[jakobbotsch]

My understanding is that an AV in a helper is only ok for helpers described by IsIPInMarkedJitHelper and CORINFO_HELP_ASSIGN_BYREF is not in this set.

Funnily enough, it looks like JIT_ByRefWriteBarrier is already in this set on x86, contained in JIT_WriteBarrierGroup. Indeed, the example throws the correct exception there (even after changing F4 to uint -- otherwise it looks like the ulong field is the first field and copying it throws the exception before entering the helper).

[AndyAyersMS]

Adding the helper to the marked set looks like a plausible fix, but before we propose that, I would like to better understand how the jit is modelling the potential for an exception on the copy.

@CarolEidt can you take a look?

[CarolEidt]

I'm not sure that I understand the question about how it is modeled. This is a "relatively" atomic operation. That is, the JIT merely loads the addresses, and calls the helper to do the assignment, so I don't think there would be any impact to the JIT's exception modeling. And it would seem to me that the only alternative to adding this helper to the IsIPInMarkedJitHelper set would be for the JIT to do null checking here, which seems to me to be the wrong answer. Am I missing something?

[AndyAyersMS]

No, that's the gist of it -- should the jit explicitly null check before calling the helper, or should the runtime be updated so that an AV in the helper should be reported as if it was an AV in the method?

Seems odd that we are hitting this case only now. It doesn't seem that uncommon to have a null reference to a class with a gc typed struct field.

[mikedn]

Is there any downside to adding that helper to IsIPInMarkedJitHelper? Seems preferable to inline null checking. Obtaining byrefs in IL normally implies a null check (e.g. ldflda) but here the byref is produced by the JIT internally due to it using that helper call. The JIT could have very well generated inline code using CORINFO_HELP_ASSIGN_REF, it only uses CORINFO_HELP_ASSIGN_BYREF as a size optimization.

[CarolEidt]

I tried making the simple change to add JIT_ByRefWriteBarrier to IsIPInMarkedJitHelper, but I get an error in the build of dactablegen:

CUSTOMBUILD : warning CS1668: Invalid search path 'lib\um\x64' specified in 'LI
B environment variable' -- 'directory does not exist' [C:\git\coreclr\bin\obj\W
indows_NT.x64.Checked\src\ToolBox\SOS\DacTableGen\dactablegen.vcxproj]
c:\git\coreclr\src\vm\excep.cpp(6980): error C2065: 'JIT_ByRefWriteBarrier_End'
: undeclared identifier [C:\git\coreclr\bin\obj\Windows_NT.x64.Checked\src\vm\w
ks\cee_wks.vcxproj]

Before I spend a lot of time tracking this down, can someone point me in the right direction? @jkotas @janvorli

[AndyAyersMS]

Try changing this:

LEAF_END JIT_ByRefWriteBarrier, _TEXT

to this

LEAF_END_MARKED JIT_ByRefWriteBarrier, _TEXT

This likely over-states the range we want to protect though....

[jkotas]

Also, the change in IsIPInMarkedJitHelper will need to be under set of platform specific ifdefs to make it compile everywhere. The assembly code for each platform is done differently here.

[CarolEidt]

AFAICT every target except for amd64 classifies this barrier exit/end (often via a generic barrier exit macro) as marked.

@jkotas - I'm not sure what you mean by "Also, the change in IsIPInMarkedJitHelper will need to be under set of platform specific ifdefs to make it compile everywhere. The assembly code for each platform is done differently here." It seems that every platform has this helper defined, so I'm not sure how it would be different. For the most part, it seems that only X86 is different.

[jkotas]

It seems that every platform has this helper defined

Ok, I see - ignore my comment then.

[jkotas]

@CarolEidt Real world customer ASP.NET scenarios are hitting this bug - https://github.com/aspnet/Mvc/issues/8572 has the details. Could you please back-port the fix to 2.2 (or even 2.1 if it meets the bar)?

cc @austindrenski

[AndyAyersMS]

@CarolEidt this is all fixed now, right?

[CarolEidt]

@AndyAyersMS - yes this was fixed in dotnet/coreclr#19571 but reopened for back-ported fixes. It was accepted & merged for 2.2 but not 2.1. I think this can be closed now.