Improve stack limit checking precision

This change improves the precision of checking whether an address is in stack limits
for cases where ulimit -s is set to unlimited and the stack limit is checked for the
current thread. It uses current SP instead of the cached stack limit, since the OS
is free to move the stack limit up during the process runtime and the cached value
reflects the initial stack limit.

6 files changed, 28 insertions, 30 deletions

diff --git a/src/vm/exceptionhandling.cpp b/src/vm/exceptionhandling.cpp
index 4441f3b039..3f89e41c71 100644
--- a/src/vm/exceptionhandling.cpp
+++ b/src/vm/exceptionhandling.cpp
@@ -4330,20 +4330,6 @@ static void DoEHLog(
 #ifdef FEATURE_PAL
 //---------------------------------------------------------------------------------------
 //
-// This functions return True if the given stack address is
-// within the specified stack boundaries.
-//
-// Arguments:
-//      sp - a stack pointer that needs to be verified
-//      stackLowAddress, stackHighAddress - these values specify stack boundaries
-//
-bool IsSpInStackLimits(ULONG64 sp, ULONG64 stackLowAddress, ULONG64 stackHighAddress)
-{
-    return ((sp > stackLowAddress) && (sp < stackHighAddress));
-}
-
-//---------------------------------------------------------------------------------------
-//
 // This function initiates unwinding of native frames during the unwinding of a managed 
 // exception. The managed exception can be propagated over several managed / native ranges 
 // until it is finally handled by a managed handler or leaves the stack unhandled and
@@ -4378,8 +4364,6 @@ VOID UnwindManagedExceptionPass2(PAL_SEHException& ex, CONTEXT* unwindStartConte
     EECodeInfo codeInfo;
     UINT_PTR establisherFrame = NULL;
     PVOID handlerData;
-    ULONG64 stackHighAddress = (ULONG64)PAL_GetStackBase();
-    ULONG64 stackLowAddress = (ULONG64)PAL_GetStackLimit();
 
     // Indicate that we are performing second pass.
     ex.ExceptionRecord.ExceptionFlags = EXCEPTION_UNWINDING;
@@ -4419,8 +4403,7 @@ VOID UnwindManagedExceptionPass2(PAL_SEHException& ex, CONTEXT* unwindStartConte
             // Make sure that the establisher frame pointer is within stack boundaries
             // and we did not go below that target frame.
             // TODO: make sure the establisher frame is properly aligned.
-            if (!IsSpInStackLimits(establisherFrame, stackLowAddress, stackHighAddress) ||
-                establisherFrame > ex.TargetFrameSp)
+            if (!Thread::IsAddressInCurrentStack((void*)establisherFrame) || establisherFrame > ex.TargetFrameSp)
             {
                 // TODO: add better error handling
                 UNREACHABLE();
@@ -4490,8 +4473,7 @@ VOID UnwindManagedExceptionPass2(PAL_SEHException& ex, CONTEXT* unwindStartConte
             UNREACHABLE();
         }
 
-    } while (IsSpInStackLimits(GetSP(currentFrameContext), stackLowAddress, stackHighAddress) &&
-        (establisherFrame != ex.TargetFrameSp));
+    } while (Thread::IsAddressInCurrentStack((void*)GetSP(currentFrameContext)) && (establisherFrame != ex.TargetFrameSp));
 
     _ASSERTE(!"UnwindManagedExceptionPass2: Unwinding failed. Reached the end of the stack");
     EEPOLICY_HANDLE_FATAL_ERROR(COR_E_EXECUTIONENGINE);
@@ -4520,8 +4502,6 @@ VOID DECLSPEC_NORETURN UnwindManagedExceptionPass1(PAL_SEHException& ex)
     UINT_PTR controlPc;
     UINT_PTR establisherFrame = NULL;
     PVOID handlerData;
-    ULONG64 stackHighAddress = (ULONG64)PAL_GetStackBase();
-    ULONG64 stackLowAddress = (ULONG64)PAL_GetStackLimit();
 
 #ifdef FEATURE_HIJACK
     GetThread()->UnhijackThread();
@@ -4569,7 +4549,7 @@ VOID DECLSPEC_NORETURN UnwindManagedExceptionPass1(PAL_SEHException& ex)
 
             // Make sure that the establisher frame pointer is within stack boundaries.
             // TODO: make sure the establisher frame is properly aligned.
-            if (!IsSpInStackLimits(establisherFrame, stackLowAddress, stackHighAddress))
+            if (!Thread::IsAddressInCurrentStack((void*)establisherFrame))
             {
                 // TODO: add better error handling
                 UNREACHABLE();
@@ -4652,7 +4632,7 @@ VOID DECLSPEC_NORETURN UnwindManagedExceptionPass1(PAL_SEHException& ex)
             }
         }
 
-    } while (IsSpInStackLimits(GetSP(&frameContext), stackLowAddress, stackHighAddress));
+    } while (Thread::IsAddressInCurrentStack((void*)GetSP(&frameContext)));
 
     _ASSERTE(!"UnwindManagedExceptionPass1: Failed to find a handler. Reached the end of the stack");
     EEPOLICY_HANDLE_FATAL_ERROR(COR_E_EXECUTIONENGINE);
diff --git a/src/vm/frames.h b/src/vm/frames.h
index 1030bc2777..68ed9d6d9c 100644
--- a/src/vm/frames.h
+++ b/src/vm/frames.h
@@ -3837,7 +3837,7 @@ class FrameWithCookieHolder
 #endif // #ifndef DACCESS_COMPILE
 
 
-#define ASSERT_ADDRESS_IN_STACK(address) _ASSERTE (GetThread () && GetThread ()->IsAddressInStack (address));
+#define ASSERT_ADDRESS_IN_STACK(address) _ASSERTE (Thread::IsAddressInCurrentStack (address));
 
 #if defined (_DEBUG) && !defined (DACCESS_COMPILE)
 #define ASSUME_BYREF_FROM_JIT_STACK_BEGIN(__objRef)                                      \
diff --git a/src/vm/qcall.h b/src/vm/qcall.h
index 84b67c7f24..893f5f09c8 100644
--- a/src/vm/qcall.h
+++ b/src/vm/qcall.h
@@ -187,7 +187,7 @@ public:
             CONTRACTL_END;
 
             // The space for the return value has to be on the stack
-            _ASSERTE(GetThread()->IsAddressInStack(m_ppStringObject));
+            _ASSERTE(Thread::IsAddressInCurrentStack(m_ppStringObject));
 
             *m_ppStringObject = STRINGREFToObject(s);
         }
@@ -214,7 +214,7 @@ public:
             LIMITED_METHOD_CONTRACT;
 
             // The space for the return value has to be on the stack
-            _ASSERTE(GetThread()->IsAddressInStack(m_ppObject));
+            _ASSERTE(Thread::IsAddressInCurrentStack(m_ppObject));
 
             *m_ppObject = OBJECTREFToObject(o);
         }
diff --git a/src/vm/reflectioninvocation.cpp b/src/vm/reflectioninvocation.cpp
index d3a3125ed0..ec5a1b1017 100644
--- a/src/vm/reflectioninvocation.cpp
+++ b/src/vm/reflectioninvocation.cpp
@@ -2989,7 +2989,7 @@ FCIMPLEND
 FCIMPL1(FC_BOOL_RET, ReflectionInvocation::IsAddressInStack, void * ptr)
 {
     FCALL_CONTRACT;
-    FC_RETURN_BOOL(GetThread()->IsAddressInStack(ptr));
+    FC_RETURN_BOOL(Thread::IsAddressInCurrentStack(ptr));
 }
 FCIMPLEND
 #endif
diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp
index 22e3b88d79..f7e125137d 100644
--- a/src/vm/threads.cpp
+++ b/src/vm/threads.cpp
@@ -7924,7 +7924,6 @@ BOOL Thread::CanResetStackTo(LPCVOID stackPointer)
         return FALSE;
     }
 }
-#endif // FEATURE_STACK_PROBE
 
 /*
  * IsStackSpaceAvailable
@@ -7975,6 +7974,8 @@ BOOL Thread::IsStackSpaceAvailable(float numPages)
     return TRUE;
 }
 
+#endif // FEATURE_STACK_PROBE
+
 /*
  * GetStackGuarantee
  *
diff --git a/src/vm/threads.h b/src/vm/threads.h
index da94c0e2ce..179f9f4e8a 100644
--- a/src/vm/threads.h
+++ b/src/vm/threads.h
@@ -198,6 +198,7 @@ class Thread
 
 public:
     BOOL IsAddressInStack (PTR_VOID addr) const { return TRUE; }
+    static BOOL IsAddressInCurrentStack (PTR_VOID addr) { return TRUE; }
 
     Frame *IsRunningIn(AppDomain* pDomain, int *count) { return NULL; }
 
@@ -3853,6 +3854,21 @@ public:
         return m_CacheStackLimit < addr && addr <= m_CacheStackBase;
     }
 
+    static BOOL IsAddressInCurrentStack (PTR_VOID addr)
+    {
+        LIMITED_METHOD_DAC_CONTRACT;
+        Thread* currentThread = GetThread();
+        if (currentThread == NULL)
+        {
+            return FALSE;
+        }
+
+        PTR_VOID sp = dac_cast<PTR_VOID>(GetCurrentSP());
+        _ASSERTE(currentThread->m_CacheStackBase != NULL);
+        _ASSERTE(sp < currentThread->m_CacheStackBase);
+        return sp < addr && addr <= currentThread->m_CacheStackBase;
+    }
+
     // DetermineIfGuardPagePresent returns TRUE if the thread's stack contains a proper guard page. This function
     // makes a physical check of the stack, rather than relying on whether or not the CLR is currently processing a
     // stack overflow exception.
@@ -3862,10 +3878,11 @@ public:
     // CanResetStackTo will return TRUE if the given stack pointer is far enough away from the guard page to proper
     // restore the guard page with RestoreGuardPage.
     BOOL CanResetStackTo(LPCVOID stackPointer);
-#endif
 
     // IsStackSpaceAvailable will return true if there are the given number of stack pages available on the stack.
     BOOL IsStackSpaceAvailable(float numPages);
+
+#endif
     
     // Returns the amount of stack available after an SO but before the OS rips the process.
     static UINT_PTR GetStackGuarantee();