From 445f01d064063fceb3840dfdf4fdf796585fde7a Mon Sep 17 00:00:00 2001
From: Jan Vorlicek <janvorli@microsoft.com>
Date: Thu, 24 Aug 2023 22:46:38 +0200
Subject: [PATCH] Fix stack_limit handling (#90937)

* Fix stack_limit handling

There is a problem with computing stack_limit in the presence of
inactive InlinedCallFrame. We were trying to call GetCallSiteSP on that
frame to get the call site SP. But when the inlined call frame is not
active (there is no call to native code in progress), that method
returns random junk.

But even if we checked for an active call before reading the value, it
would not get correct stack limit on x86 windows when there was no
active call with the inlined call frame being the topmost one. That
can happen with hardware exception handling on Windows x86. On other
targets, we always have other explicit frame on top of the explicit
frames stack, but on windows x86, we don't use the FaultingExceptionFrame for
hardware exceptions, so the inactive inlined call frame could be the
topmost one when GC starts to scan stack roots.

Since the stack_limit was introduced to fix a Unix specific problem, I
have fixed that by disabling the stack_limit usage for x86 windows.

Close #86265

* Add back the stack_limit field to keep contract unchanged

* Reflect PR feedback - Minimalize the number of ifdefs

* Reflect PR feedback
---
 src/coreclr/vm/gcenv.ee.cpp | 18 +++++++++++++++---
 src/coreclr/vm/siginfo.cpp  |  5 -----
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/src/coreclr/vm/gcenv.ee.cpp b/src/coreclr/vm/gcenv.ee.cpp
index dc28e791de0cf..8d9f676d967a8 100644
--- a/src/coreclr/vm/gcenv.ee.cpp
+++ b/src/coreclr/vm/gcenv.ee.cpp
@@ -115,16 +115,28 @@ static void ScanStackRoots(Thread * pThread, promote_func* fn, ScanContext* sc)
                 IsGCSpecialThread() ||
                 (GetThread() == ThreadSuspend::GetSuspensionThread() && ThreadStore::HoldingThreadStore()));
 
+#if defined(FEATURE_CONSERVATIVE_GC) || defined(USE_FEF)
     Frame* pTopFrame = pThread->GetFrame();
     Object ** topStack = (Object **)pTopFrame;
-    if ((pTopFrame != ((Frame*)-1))
-        && (pTopFrame->GetVTablePtr() == InlinedCallFrame::GetMethodFrameVPtr())) {
-        // It is an InlinedCallFrame. Get SP from it.
+    if (InlinedCallFrame::FrameHasActiveCall(pTopFrame))
+    {
+        // It is an InlinedCallFrame with active call. Get SP from it.
         InlinedCallFrame* pInlinedFrame = (InlinedCallFrame*)pTopFrame;
         topStack = (Object **)pInlinedFrame->GetCallSiteSP();
     }
+#endif // FEATURE_CONSERVATIVE_GC || USE_FEF
 
+#ifdef USE_FEF
+    // We only set the stack_limit when FEF (FaultingExceptionFrame) is enabled, because without the
+    // FEF, the code above would have to check if hardware exception is being handled and get the limit
+    // from the exception frame. Since the stack_limit is strictly necessary only on Unix and FEF is
+    // not enabled on Window x86 only, it is sufficient to keep the stack_limit set to 0 in this case.
+    // See the comment on the stack_limit usage in the PromoteCarefully function for more details.
     sc->stack_limit = (uintptr_t)topStack;
+#else // USE_FEF
+    // It should be set to 0 in the ScanContext constructor
+    _ASSERTE(sc->stack_limit == 0);
+#endif // USE_FEF
 
 #ifdef FEATURE_CONSERVATIVE_GC
     if (g_pConfig->GetGCConservative())
diff --git a/src/coreclr/vm/siginfo.cpp b/src/coreclr/vm/siginfo.cpp
index b9807b51ba4af..eb35f3a372538 100644
--- a/src/coreclr/vm/siginfo.cpp
+++ b/src/coreclr/vm/siginfo.cpp
@@ -4962,11 +4962,6 @@ void PromoteCarefully(promote_func   fn,
 
 #if !defined(DACCESS_COMPILE)
 
-    //
-    // Sanity check the stack scan limit
-    //
-    assert(sc->stack_limit != 0);
-
     // Note that the base is at a higher address than the limit, since the stack
     // grows downwards.
     // To check whether the object is in the stack or not, we also need to check the sc->stack_limit.