From c29b30bc4c7bf976db6fcf2c44e352920ace63aa Mon Sep 17 00:00:00 2001
From: Eugene Rozenfeld <erozen@microsoft.com>
Date: Thu, 12 Apr 2018 16:05:44 -0700
Subject: Fix for 17398. (#17501)

When enumerating live gc registers, if we are not on the active stack frame,
we need to report callee-save gc registers that are live before the call.
The reason is that the liveness of gc registers may change across a call
to a method that does not return. In this case the instruction after the call
may be a jump target and a register that didn't have a live gc pointer before
the call may have a live gc pointer after the jump. To make sure we report the
registers that have live gc pointers before the call we subtract 1 from curOffs.
---
 src/vm/eetwain.cpp | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

(limited to 'src/vm/eetwain.cpp')

diff --git a/src/vm/eetwain.cpp b/src/vm/eetwain.cpp
index 1fb332ecd4..511a635509 100644
--- a/src/vm/eetwain.cpp
+++ b/src/vm/eetwain.cpp
@@ -2404,14 +2404,16 @@ FINISHED:
    Returns size of things pushed on the stack for ESP frames
 
    Arguments:
-      table   - The pointer table
-      curOffs - The current code offset
-      info    - Incoming arg used to determine if there's a frame, and to save results
+      table       - The pointer table
+      curOffsRegs - The current code offset that should be used for reporting registers
+      curOffsArgs - The current code offset that should be used for reporting args
+      info        - Incoming arg used to determine if there's a frame, and to save results
  */
 
 static
-unsigned scanArgRegTableI(PTR_CBYTE     table,
-                          unsigned     curOffs,
+unsigned scanArgRegTableI(PTR_CBYTE    table,
+                          unsigned     curOffsRegs,
+                          unsigned     curOffsArgs,
                           hdrInfo   *  info)
 {
     CONTRACTL {
@@ -2433,6 +2435,10 @@ unsigned scanArgRegTableI(PTR_CBYTE     table,
     bool      isThis     = false;
     bool      iptr       = false;
 
+    // The comment before the call to scanArgRegTableI in EnumGCRefs
+    // describes why curOffsRegs can be smaller than curOffsArgs.
+    _ASSERTE(curOffsRegs <= curOffsArgs);
+
 #if VERIFY_GC_TABLES
     _ASSERTE(*castto(table, unsigned short *)++ == 0xBABE);
 #endif
@@ -2506,7 +2512,7 @@ unsigned scanArgRegTableI(PTR_CBYTE     table,
 
     /* Have we reached the instruction we're looking for? */
 
-    while (ptrOffs <= curOffs)
+    while (ptrOffs <= curOffsArgs)
     {
         unsigned    val;
 
@@ -2543,10 +2549,14 @@ unsigned scanArgRegTableI(PTR_CBYTE     table,
             regNum       reg;
 
             ptrOffs += (val     ) & 0x7;
-            if (ptrOffs > curOffs) {
+            if (ptrOffs > curOffsArgs) {
                 iptr = isThis = false;
                 goto REPORT_REFS;
             }
+            else if (ptrOffs > curOffsRegs) {
+                iptr = isThis = false;
+                continue;
+            }
 
             reg     = (regNum)((val >> 3) & 0x7);
             regMask = 1 << reg;         // EAX,ECX,EDX,EBX,---,EBP,ESI,EDI
@@ -2606,7 +2616,7 @@ unsigned scanArgRegTableI(PTR_CBYTE     table,
             /* A small argument encoding */
 
             ptrOffs += (val & 0x07);
-            if (ptrOffs > curOffs) {
+            if (ptrOffs > curOffsArgs) {
                 iptr = isThis = false;
                 goto REPORT_REFS;
             }
@@ -2736,7 +2746,7 @@ unsigned scanArgRegTableI(PTR_CBYTE     table,
                 /* non-ptr arg push */
                 _ASSERTE(!hasPartialArgInfo);
                 ptrOffs += (val & 0x07);
-                if (ptrOffs > curOffs) {
+                if (ptrOffs > curOffsArgs) {
                     iptr = isThis = false;
                     goto REPORT_REFS;
                 }
@@ -2858,6 +2868,7 @@ unsigned GetPushedArgSize(hdrInfo * info, PTR_CBYTE table, DWORD curOffs)
     if  (info->interruptible)
     {
         sz = scanArgRegTableI(skipToArgReg(*info, table),
+                              curOffs,
                               curOffs,
                               info);
     }
@@ -4436,7 +4447,15 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY     pContext,
 
     if  (info.interruptible)
     {
-        pushedSize = scanArgRegTableI(skipToArgReg(info, table), curOffs, &info);
+        // If we are not on the active stack frame, we need to report gc registers
+        // that are live before the call. The reason is that the liveness of gc registers
+        // may change across a call to a method that does not return. In this case the instruction
+        // after the call may be a jump target and a register that didn't have a live gc pointer
+        // before the call may have a live gc pointer after the jump. To make sure we report the
+        // registers that have live gc pointers before the call we subtract 1 from curOffs.
+        unsigned curOffsRegs = (flags & ActiveStackFrame) != 0 ? curOffs : curOffs - 1;
+
+        pushedSize = scanArgRegTableI(skipToArgReg(info, table), curOffsRegs, curOffs, &info);
 
         RegMask   regs  = info.regMaskResult;
         RegMask  iregs  = info.iregMaskResult;
@@ -5342,7 +5361,7 @@ OBJECTREF EECodeManager::GetInstance( PREGDISPLAY    pContext,
 
     if  (info.interruptible)
     {
-        stackDepth = scanArgRegTableI(skipToArgReg(info, table), (unsigned)relOffset, &info);
+        stackDepth = scanArgRegTableI(skipToArgReg(info, table), relOffset, relOffset, &info);
     }
     else
     {
@@ -6046,6 +6065,7 @@ TADDR EECodeManager::GetAmbientSP(PREGDISPLAY     pContext,
     if  (stateBuf->hdrInfoBody.interruptible)
     {
         baseSP += scanArgRegTableI(skipToArgReg(stateBuf->hdrInfoBody, table),
+                                   dwRelOffset,
                                    dwRelOffset,
                                    &stateBuf->hdrInfoBody);
     }
-- 
cgit v1.2.3