New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jakobbotsch opened this Issue Aug 12, 2018 · 14 comments

Comments

Projects
None yet
6 participants
@jakobbotsch
Collaborator

jakobbotsch commented Aug 12, 2018

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

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Aug 13, 2018

Member

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

Member

danmosemsft commented Aug 13, 2018

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

@AndyAyersMS AndyAyersMS added this to the 3.0 milestone Aug 14, 2018

@AndyAyersMS

This comment has been minimized.

Show comment
Hide comment
@AndyAyersMS

AndyAyersMS Aug 14, 2018

Member

I'll investigate.

Member

AndyAyersMS commented Aug 14, 2018

I'll investigate.

@AndyAyersMS AndyAyersMS self-assigned this Aug 14, 2018

@AndyAyersMS

This comment has been minimized.

Show comment
Hide comment
@AndyAyersMS

AndyAyersMS Aug 15, 2018

Member

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.

Member

AndyAyersMS commented Aug 15, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Aug 15, 2018

Member

helpers described by IsIPInMarkedJitHelper

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

Member

jkotas commented Aug 15, 2018

helpers described by IsIPInMarkedJitHelper

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

@jakobbotsch

This comment has been minimized.

Show comment
Hide comment
@jakobbotsch

jakobbotsch Aug 15, 2018

Collaborator

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).

Collaborator

jakobbotsch commented Aug 15, 2018

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

This comment has been minimized.

Show comment
Hide comment
@AndyAyersMS

AndyAyersMS Aug 15, 2018

Member

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?

Member

AndyAyersMS commented Aug 15, 2018

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

This comment has been minimized.

Show comment
Hide comment
@CarolEidt

CarolEidt Aug 18, 2018

Member

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?

Member

CarolEidt commented Aug 18, 2018

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

This comment has been minimized.

Show comment
Hide comment
@AndyAyersMS

AndyAyersMS Aug 18, 2018

Member

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.

Member

AndyAyersMS commented Aug 18, 2018

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

This comment has been minimized.

Show comment
Hide comment
@mikedn

mikedn Aug 18, 2018

Contributor

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.

Contributor

mikedn commented Aug 18, 2018

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

This comment has been minimized.

Show comment
Hide comment
@CarolEidt

CarolEidt Aug 20, 2018

Member

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

Member

CarolEidt commented Aug 20, 2018

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

This comment has been minimized.

Show comment
Hide comment
@AndyAyersMS

AndyAyersMS Aug 20, 2018

Member

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....

Member

AndyAyersMS commented Aug 20, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Aug 20, 2018

Member

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.

Member

jkotas commented Aug 20, 2018

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

This comment has been minimized.

Show comment
Hide comment
@CarolEidt

CarolEidt Aug 20, 2018

Member

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.

Member

CarolEidt commented Aug 20, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Aug 20, 2018

Member

It seems that every platform has this helper defined

Ok, I see - ignore my comment then.

Member

jkotas commented Aug 20, 2018

It seems that every platform has this helper defined

Ok, I see - ignore my comment then.

CarolEidt added a commit to CarolEidt/coreclr that referenced this issue Aug 20, 2018

Handle null byref in helper
Add JIT_ByRefWriteBarrier to IsIPInMarkedJitHelper so that a null dereference will be handled.

Fix #19444

@jkotas jkotas closed this in #19571 Aug 21, 2018

jkotas added a commit that referenced this issue Aug 21, 2018

Handle null byref in helper (#19571)
Add JIT_ByRefWriteBarrier to IsIPInMarkedJitHelper so that a null dereference will be handled.

Fix #19444
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment