From 20a2b04e8f7d1cc7bf85332966d0f252e0cc3d3b Mon Sep 17 00:00:00 2001
From: Diego Russo <diego.russo@arm.com>
Date: Thu, 28 Mar 2024 11:14:12 +0000
Subject: [PATCH] gh-106883 Fix deadlock in threaded application

When using threaded applications, there is a high risk of a deadlock in
the intepreter.
It's a lock ordering deadlock with HEAD_LOCK(&_PyRuntime); and the GIL.

By disabling the GC during the _PyThread_CurrentFrames() and
_PyThread_CurrentExceptions() calls fixes the issue.
---
 Lib/test/test_sys.py | 49 ++++++++++++++++++++++++++++++++++++++++++++
 Python/pystate.c     | 27 ++++++++++++------------
 2 files changed, 63 insertions(+), 13 deletions(-)

diff --git a/Lib/test/test_sys.py b/Lib/test/test_sys.py
index 86cf1a794f973c..7bed9440e5c20b 100644
--- a/Lib/test/test_sys.py
+++ b/Lib/test/test_sys.py
@@ -471,6 +471,55 @@ def g456():
         leave_g.set()
         t.join()
 
+    @threading_helper.reap_threads
+    @threading_helper.requires_working_threading()
+    @support.requires_fork()
+    def test_current_frames_exceptions_deadlock(self):
+        """
+        Try to reproduce the bug raised in GH-106883 and GH-116969.
+        """
+        import threading
+        import time
+
+        class MockObject:
+            def __init__(self):
+                # Create some garbage
+                self._list = list(range(10000))
+                # Call the functions under test
+                self._trace = sys._current_frames()
+                self._exceptions = sys._current_exceptions()
+
+        def thread_function(num_objects):
+            obj = None
+            for _ in range(num_objects):
+                # The sleep is needed to have a syscall: in interrupts the
+                # current thread, releases the GIL and gives way to other
+                # threads to be executed. In this way there are more chances
+                # to reproduce the bug.
+                time.sleep(0)
+                obj = MockObject()
+
+        NUM_OBJECTS = 25
+        NUM_THREADS = 1000
+
+        # 60 seconds should be enough for the test to be executed: if it
+        # is more than 60 seconds it means that the process is in deadlock
+        # hence the test fails
+        TIMEOUT = 60
+
+        # Test the sys._current_frames and sys._current_exceptions calls
+        pid = os.fork()
+        if pid:  # parent process
+            support.wait_process(pid, exitcode=0, timeout=TIMEOUT)
+        else:  # child process
+            # Run the actual test in the forked process.
+            for i in range(NUM_THREADS):
+                thread = threading.Thread(
+                    target=thread_function, args=(NUM_OBJECTS,)
+                )
+                thread.start()
+            os._exit(0)
+
     @threading_helper.reap_threads
     @threading_helper.requires_working_threading()
     def test_current_exceptions(self):
diff --git a/Python/pystate.c b/Python/pystate.c
index 264191f943e81d..a45116665fe195 100644
--- a/Python/pystate.c
+++ b/Python/pystate.c
@@ -2,7 +2,6 @@
 /* Thread and interpreter state structures and their interfaces */
 
 #include "Python.h"
-#include "objimpl.h"
 #include "pycore_ceval.h"
 #include "pycore_code.h"           // stats
 #include "pycore_frame.h"
@@ -1389,10 +1388,6 @@ PyThreadState_Next(PyThreadState *tstate) {
 PyObject *
 _PyThread_CurrentFrames(void)
 {
-    // Disable the GC as this can cause a deadlock the interpreter.
-    // See issues 116969 and 106883.
-    PyGC_Disable();
-
     PyThreadState *tstate = _PyThreadState_GET();
     if (_PySys_Audit(tstate, "sys._current_frames", NULL) < 0) {
         return NULL;
@@ -1403,6 +1398,9 @@ _PyThread_CurrentFrames(void)
         return NULL;
     }
 
+    // gh-106883: Disable the GC as this can cause the interpreter to deadlock
+    int gc_was_enabled = PyGC_Disable();
+
     /* for i in all interpreters:
      *     for t in all of i's thread states:
      *          if t's frame isn't NULL, map t's id to its frame
@@ -1446,8 +1444,10 @@ _PyThread_CurrentFrames(void)
 done:
     HEAD_UNLOCK(runtime);
 
-    // Once we release the runtime, the GC can be reenabled.
-    PyGC_Enable();
+    // Once the runtime is released, the GC can be reenabled.
+    if (gc_was_enabled) {
+        PyGC_Enable();
+    }
 
     return result;
 }
@@ -1455,10 +1455,6 @@ _PyThread_CurrentFrames(void)
 PyObject *
 _PyThread_CurrentExceptions(void)
 {
-    // Disable the GC as this can cause a deadlock the interpreter.
-    // See issues 116969 and 106883.
-    PyGC_Disable();
-
     PyThreadState *tstate = _PyThreadState_GET();
 
     _Py_EnsureTstateNotNULL(tstate);
@@ -1472,6 +1468,9 @@ _PyThread_CurrentExceptions(void)
         return NULL;
     }
 
+    // gh-106883: Disable the GC as this can cause the interpreter to deadlock
+    int gc_was_enabled = PyGC_Disable();
+
     /* for i in all interpreters:
      *     for t in all of i's thread states:
      *          if t's frame isn't NULL, map t's id to its frame
@@ -1513,8 +1512,10 @@ _PyThread_CurrentExceptions(void)
 done:
     HEAD_UNLOCK(runtime);
 
-    // Once we release the runtime, the GC can be reenabled.
-    PyGC_Enable();
+    // Once the runtime is released, the GC can be reenabled.
+    if (gc_was_enabled) {
+        PyGC_Enable();
+    }
 
     return result;
 }