Port from tfs: onload exception debugger crash fix (#4868)

The ExceptionHijackPersonalityRoutine was retrieving a context from a
particular stack frame, but the calling conventions for the function
executing in that frame allow it to overwrite the context. This was
causing the debugger to crash for exceptions thrown from the onload
method in winforms on x64.

The fix is to instead retrieve the context from the previous stack
frame, whose layout is set up explicitly in ExceptionHijack.

diff --git a/src/debug/ee/amd64/dbghelpers.S b/src/debug/ee/amd64/dbghelpers.S
index 405c6a5271c91b9f25f692f612d661af1d2b19d0..85ec80c7014ff6bc33d35b046a0535d04fcb08dc 100644 (file)
--- a/src/debug/ee/amd64/dbghelpers.S
+++ b/src/debug/ee/amd64/dbghelpers.S
@@ -45,6 +45,13 @@ NESTED_ENTRY ExceptionHijack, _TEXT, UnhandledExceptionHandlerUnix
         // So we allocate 4 stack slots as the outgoing argument home and just copy the 
         // arguments set up by DacDbi into these stack slots.  We will take a perf hit,
         // but this is not a perf critical code path anyway.
+
+        // There is an additional dependency on this alloc_stack: the
+        // ExceptionHijackPersonalityRoutine assumes that it can find
+        // the first argument to HijackWorker in the stack frame of
+        // ExceptionHijack, at an offset of exactly 0x20 bytes from
+        // ExceptionHijackWorker's stack frame. Therefore it is
+        // important that we move the stack pointer by the same amount.
         alloc_stack 0x20
         END_PROLOGUE
 
@@ -53,6 +60,11 @@ NESTED_ENTRY ExceptionHijack, _TEXT, UnhandledExceptionHandlerUnix
         // stack for us.  However, the Orcas compilers don't like a 0-sized frame, so 
         // we need to allocate something here and then just copy the stack arguments to 
         // their new argument homes.
+
+        // In x86, ExceptionHijackWorker is marked STDCALL, so it finds
+        // its arguments on the stack. In x64, it gets its arguments in
+        // registers (set up for us by DacDbiInterfaceImpl::Hijack),
+        // and this stack space may be reused.
         mov     rax, [rsp + 20h]
         mov     [rsp], rax
         mov     rax, [rsp + 28h]

diff --git a/src/debug/ee/amd64/dbghelpers.asm b/src/debug/ee/amd64/dbghelpers.asm
index 9393552b7aaa7a897dbd5233288ce234f6263fad..5836257f465abc595d35f8bc778ba41257216a68 100644 (file)
--- a/src/debug/ee/amd64/dbghelpers.asm
+++ b/src/debug/ee/amd64/dbghelpers.asm
@@ -48,6 +48,13 @@ NESTED_ENTRY ExceptionHijack, _TEXT, ExceptionHijackPersonalityRoutine
         ; So we allocate 4 stack slots as the outgoing argument home and just copy the 
         ; arguments set up by DacDbi into these stack slots.  We will take a perf hit,
         ; but this is not a perf critical code path anyway.
+
+        ; There is an additional dependency on this alloc_stack: the
+        ; ExceptionHijackPersonalityRoutine assumes that it can find
+        ; the first argument to HijackWorker in the stack frame of
+        ; ExceptionHijack, at an offset of exactly 0x20 bytes from
+        ; ExceptionHijackWorker's stack frame. Therefore it is
+        ; important that we move the stack pointer by the same amount.
         alloc_stack 20h
         END_PROLOGUE
 
@@ -56,6 +63,11 @@ NESTED_ENTRY ExceptionHijack, _TEXT, ExceptionHijackPersonalityRoutine
         ; stack for us.  However, the Orcas compilers don't like a 0-sized frame, so 
         ; we need to allocate something here and then just copy the stack arguments to 
         ; their new argument homes.
+
+        ; In x86, ExceptionHijackWorker is marked STDCALL, so it finds
+        ; its arguments on the stack. In x64, it gets its arguments in
+        ; registers (set up for us by DacDbiInterfaceImpl::Hijack),
+        ; and this stack space may be reused.
         mov     rax, [rsp + 20h]
         mov     [rsp], rax
         mov     rax, [rsp + 28h]

diff --git a/src/debug/ee/debugger.cpp b/src/debug/ee/debugger.cpp
index 12f87e381ccd345e686ed66439c0e77f1fb708d4..eb5e65c41575037fbded67c2595961ca148701fd 100644 (file)
--- a/src/debug/ee/debugger.cpp
+++ b/src/debug/ee/debugger.cpp
@@ -13502,7 +13502,17 @@ ExceptionHijackPersonalityRoutine(IN     PEXCEPTION_RECORD   pExceptionRecord
     CONTEXT * pHijackContext = NULL;
 
     // Get the 1st parameter (the Context) from hijack worker.
-    pHijackContext = *reinterpret_cast<CONTEXT **>(pDispatcherContext->EstablisherFrame);
+    // EstablisherFrame points to the stack slot 8 bytes above the
+    // return address to the ExceptionHijack. This would contain the
+    // parameters passed to ExceptionHijackWorker, which is marked
+    // STDCALL, but the x64 calling convention lets the
+    // ExceptionHijackWorker use that stack space, resulting in the
+    // context being overwritten. Instead, we get the context from the
+    // previous stack frame, which contains the arguments to
+    // ExceptionHijack, placed there by the debugger in
+    // DacDbiInterfaceImpl::Hijack. This works because ExceptionHijack
+    // allocates exactly 4 stack slots.
+    pHijackContext = *reinterpret_cast<CONTEXT **>(pDispatcherContext->EstablisherFrame + 0x20);
     
     // This copies pHijackContext into pDispatcherContext, which the OS can then
     // use to walk the stack.