pythongh-108512: Fix possible crashes related to PySys_GetObject() in…

… free-threaded build The use of PySys_GetObject() and _PySys_GetAttr(), which return a borrowed reference, has been replaced by using one of the following functions, which return a strong reference and distinguish a missing attribute from an error: _PySys_GetOptionalAttr(), _PySys_GetOptionalAttrString(), _PySys_GetRequiredAttr(), and _PySys_GetRequiredAttrString().
serhiy-storchaka · Feb 24, 2025 · e76228a · e76228a
1 parent de13293
commit e76228a
Show file tree

Hide file tree

Showing 20 changed files with 462 additions and 209 deletions.
diff --git a/Include/internal/pycore_sysmodule.h b/Include/internal/pycore_sysmodule.h
@@ -8,8 +8,10 @@ extern "C" {
 #  error "this header requires Py_BUILD_CORE define"
 #endif
 
-// Export for '_pickle' shared extension
-PyAPI_FUNC(PyObject*) _PySys_GetAttr(PyThreadState *tstate, PyObject *name);
+PyAPI_FUNC(int) _PySys_GetOptionalAttr(PyObject *, PyObject **);
+PyAPI_FUNC(int) _PySys_GetOptionalAttrString(const char *, PyObject **);
+PyAPI_FUNC(PyObject *) _PySys_GetRequiredAttr(PyObject *);
+PyAPI_FUNC(PyObject *) _PySys_GetRequiredAttrString(const char *);
 
 // Export for '_pickle' shared extension
 PyAPI_FUNC(size_t) _PySys_GetSizeOf(PyObject *);

diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-02-24-14-25-36.gh-issue-130163.rGpc9v.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-02-24-14-25-36.gh-issue-130163.rGpc9v.rst
@@ -0,0 +1,2 @@
+Fix possible crashes in a free-threaded debug build related to concurrent
+change and use of the :mod:`sys` module attributes.
diff --git a/Modules/_cursesmodule.c b/Modules/_cursesmodule.c
@@ -108,6 +108,7 @@ static const char PyCursesVersion[] = "2.2";
 #include "pycore_capsule.h"     // _PyCapsule_SetTraverse()
 #include "pycore_long.h"        // _PyLong_GetZero()
 #include "pycore_structseq.h"   // _PyStructSequence_NewType()
+#include "pycore_sysmodule.h"   // _PySys_GetOptionalAttrString()
 
 #ifdef __hpux
 #define STRICT_SYSV_CURSES
@@ -3542,16 +3543,19 @@ _curses_setupterm_impl(PyObject *module, const char *term, int fd)
     if (fd == -1) {
         PyObject* sys_stdout;
 
-        sys_stdout = PySys_GetObject("stdout");
+        if (_PySys_GetOptionalAttrString("stdout", &sys_stdout) < 0) {
+            return NULL;
+        }
 
         if (sys_stdout == NULL || sys_stdout == Py_None) {
             cursesmodule_state *state = get_cursesmodule_state(module);
             PyErr_SetString(state->error, "lost sys.stdout");
+            Py_XDECREF(sys_stdout);
             return NULL;
         }
 
         fd = PyObject_AsFileDescriptor(sys_stdout);
-
+        Py_DECREF(sys_stdout);
         if (fd == -1) {
             return NULL;
         }

diff --git a/Modules/_pickle.c b/Modules/_pickle.c
@@ -19,7 +19,7 @@
 #include "pycore_pystate.h"           // _PyThreadState_GET()
 #include "pycore_runtime.h"           // _Py_ID()
 #include "pycore_setobject.h"         // _PySet_NextEntry()
-#include "pycore_sysmodule.h"         // _PySys_GetAttr()
+#include "pycore_sysmodule.h"         // _PySys_GetSizeOf()
 
 #include <stdlib.h>               // strtol()
 
@@ -1910,10 +1910,8 @@ whichmodule(PickleState *st, PyObject *global, PyObject *global_name, PyObject *
            __module__ can be None. If it is so, then search sys.modules for
            the module of global. */
         Py_CLEAR(module_name);
-        PyThreadState *tstate = _PyThreadState_GET();
-        modules = _PySys_GetAttr(tstate, &_Py_ID(modules));
+        modules = _PySys_GetRequiredAttr(&_Py_ID(modules));
         if (modules == NULL) {
-            PyErr_SetString(PyExc_RuntimeError, "unable to get sys.modules");
             return NULL;
         }
         if (PyDict_CheckExact(modules)) {
@@ -1923,41 +1921,48 @@ whichmodule(PickleState *st, PyObject *global, PyObject *global_name, PyObject *
                 Py_INCREF(module);
                 if (_checkmodule(module_name, module, global, dotted_path) == 0) {
                     Py_DECREF(module);
+                    Py_DECREF(modules);
                     return module_name;
                 }
                 Py_DECREF(module);
                 Py_DECREF(module_name);
                 if (PyErr_Occurred()) {
+                    Py_DECREF(modules);
                     return NULL;
                 }
             }
         }
         else {
             PyObject *iterator = PyObject_GetIter(modules);
             if (iterator == NULL) {
+                Py_DECREF(modules);
                 return NULL;
             }
             while ((module_name = PyIter_Next(iterator))) {
                 module = PyObject_GetItem(modules, module_name);
                 if (module == NULL) {
                     Py_DECREF(module_name);
                     Py_DECREF(iterator);
+                    Py_DECREF(modules);
                     return NULL;
                 }
                 if (_checkmodule(module_name, module, global, dotted_path) == 0) {
                     Py_DECREF(module);
                     Py_DECREF(iterator);
+                    Py_DECREF(modules);
                     return module_name;
                 }
                 Py_DECREF(module);
                 Py_DECREF(module_name);
                 if (PyErr_Occurred()) {
                     Py_DECREF(iterator);
+                    Py_DECREF(modules);
                     return NULL;
                 }
             }
             Py_DECREF(iterator);
         }
+        Py_DECREF(modules);
         if (PyErr_Occurred()) {
             return NULL;
         }

diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c
@@ -8,7 +8,7 @@
 #include "pycore_modsupport.h"    // _PyArg_NoKeywords()
 #include "pycore_pylifecycle.h"
 #include "pycore_pystate.h"       // _PyThreadState_SetCurrent()
-#include "pycore_sysmodule.h"     // _PySys_GetAttr()
+#include "pycore_sysmodule.h"     // _PySys_GetOptionalAttr()
 #include "pycore_time.h"          // _PyTime_FromSeconds()
 #include "pycore_weakref.h"       // _PyWeakref_GET_REF()
 
@@ -2242,9 +2242,12 @@ thread_excepthook(PyObject *module, PyObject *args)
     PyObject *exc_tb = PyStructSequence_GET_ITEM(args, 2);
     PyObject *thread = PyStructSequence_GET_ITEM(args, 3);
 
-    PyThreadState *tstate = _PyThreadState_GET();
-    PyObject *file = _PySys_GetAttr(tstate, &_Py_ID(stderr));
+    PyObject *file;
+    if (_PySys_GetOptionalAttr( &_Py_ID(stderr), &file) < 0) {
+        return NULL;
+    }
     if (file == NULL || file == Py_None) {
+        Py_XDECREF(file);
         if (thread == Py_None) {
             /* do nothing if sys.stderr is None and thread is None */
             Py_RETURN_NONE;
@@ -2261,9 +2264,6 @@ thread_excepthook(PyObject *module, PyObject *args)
             Py_RETURN_NONE;
         }
     }
-    else {
-        Py_INCREF(file);
-    }
 
     int res = thread_excepthook_file(file, exc_type, exc_value, exc_tb,
                                      thread);

diff --git a/Modules/_tkinter.c b/Modules/_tkinter.c
@@ -31,6 +31,7 @@ Copyright (C) 1994 Steen Lumholt.
 #endif
 
 #include "pycore_long.h"          // _PyLong_IsNegative()
+#include "pycore_sysmodule.h"     // _PySys_GetOptionalAttrString()
 
 #ifdef MS_WINDOWS
 #  include <windows.h>
@@ -142,18 +143,21 @@ _get_tcl_lib_path(void)
     if (already_checked == 0) {
         struct stat stat_buf;
         int stat_return_value;
+        PyObject *prefix;
 
-        PyObject *prefix = PySys_GetObject("base_prefix");  // borrowed reference
+        (void) _PySys_GetOptionalAttrString("base_prefix", &prefix);
         if (prefix == NULL) {
             return NULL;
         }
 
         /* Check expected location for an installed Python first */
         tcl_library_path = PyUnicode_FromString("\\tcl\\tcl" TCL_VERSION);
         if (tcl_library_path == NULL) {
+            Py_DECREF(prefix);
             return NULL;
         }
         tcl_library_path = PyUnicode_Concat(prefix, tcl_library_path);
+        Py_DECREF(prefix);
         if (tcl_library_path == NULL) {
             return NULL;
         }
@@ -3516,9 +3520,10 @@ PyInit__tkinter(void)
 
     /* This helps the dynamic loader; in Unicode aware Tcl versions
        it also helps Tcl find its encodings. */
-    uexe = PySys_GetObject("executable");  // borrowed reference
+    (void) _PySys_GetOptionalAttrString("executable", &uexe);
     if (uexe && PyUnicode_Check(uexe)) {   // sys.executable can be None
         cexe = PyUnicode_EncodeFSDefault(uexe);
+        Py_DECREF(uexe);
         if (cexe) {
 #ifdef MS_WINDOWS
             int set_var = 0;
@@ -3531,12 +3536,14 @@ PyInit__tkinter(void)
             if (!ret && GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
                 str_path = _get_tcl_lib_path();
                 if (str_path == NULL && PyErr_Occurred()) {
+                    Py_DECREF(cexe);
                     Py_DECREF(m);
                     return NULL;
                 }
                 if (str_path != NULL) {
                     wcs_path = PyUnicode_AsWideCharString(str_path, NULL);
                     if (wcs_path == NULL) {
+                        Py_DECREF(cexe);
                         Py_DECREF(m);
                         return NULL;
                     }
@@ -3557,6 +3564,9 @@ PyInit__tkinter(void)
         }
         Py_XDECREF(cexe);
     }
+    else {
+        Py_XDECREF(uexe);
+    }
 
     if (PyErr_Occurred()) {
         Py_DECREF(m);

diff --git a/Modules/faulthandler.c b/Modules/faulthandler.c
@@ -4,7 +4,7 @@
 #include "pycore_pyerrors.h"      // _Py_DumpExtensionModules
 #include "pycore_pystate.h"       // _PyThreadState_GET()
 #include "pycore_signal.h"        // Py_NSIG
-#include "pycore_sysmodule.h"     // _PySys_GetAttr()
+#include "pycore_sysmodule.h"     // _PySys_GetRequiredAttr()
 #include "pycore_time.h"          // _PyTime_FromSecondsObject()
 #include "pycore_traceback.h"     // _Py_DumpTracebackThreads
 
@@ -112,14 +112,13 @@ faulthandler_get_fileno(PyObject **file_ptr)
     PyObject *file = *file_ptr;
 
     if (file == NULL || file == Py_None) {
-        PyThreadState *tstate = _PyThreadState_GET();
-        file = _PySys_GetAttr(tstate, &_Py_ID(stderr));
+        file = _PySys_GetRequiredAttr(&_Py_ID(stderr));
         if (file == NULL) {
-            PyErr_SetString(PyExc_RuntimeError, "unable to get sys.stderr");
             return -1;
         }
         if (file == Py_None) {
             PyErr_SetString(PyExc_RuntimeError, "sys.stderr is None");
+            Py_DECREF(file);
             return -1;
         }
     }
@@ -142,10 +141,15 @@ faulthandler_get_fileno(PyObject **file_ptr)
         *file_ptr = NULL;
         return fd;
     }
+    else {
+        Py_INCREF(file);
+    }
 
     result = PyObject_CallMethodNoArgs(file, &_Py_ID(fileno));
-    if (result == NULL)
+    if (result == NULL) {
+        Py_DECREF(file);
         return -1;
+    }
 
     fd = -1;
     if (PyLong_Check(result)) {
@@ -158,6 +162,7 @@ faulthandler_get_fileno(PyObject **file_ptr)
     if (fd == -1) {
         PyErr_SetString(PyExc_RuntimeError,
                         "file.fileno() is not a valid file descriptor");
+        Py_DECREF(file);
         return -1;
     }
 
@@ -240,8 +245,10 @@ faulthandler_dump_traceback_py(PyObject *self,
         return NULL;
 
     tstate = get_thread_state();
-    if (tstate == NULL)
+    if (tstate == NULL) {
+        Py_XDECREF(file);
         return NULL;
+    }
 
     if (all_threads) {
         PyInterpreterState *interp = _PyInterpreterState_GET();
@@ -252,12 +259,14 @@ faulthandler_dump_traceback_py(PyObject *self,
         _PyEval_StartTheWorld(interp);
         if (errmsg != NULL) {
             PyErr_SetString(PyExc_RuntimeError, errmsg);
+            Py_XDECREF(file);
             return NULL;
         }
     }
     else {
         _Py_DumpTraceback(fd, tstate);
     }
+    Py_XDECREF(file);
 
     if (PyErr_CheckSignals())
         return NULL;
@@ -540,10 +549,11 @@ faulthandler_py_enable(PyObject *self, PyObject *args, PyObject *kwargs)
         return NULL;
 
     tstate = get_thread_state();
-    if (tstate == NULL)
+    if (tstate == NULL) {
+        Py_XDECREF(file);
         return NULL;
+    }
 
-    Py_XINCREF(file);
     Py_XSETREF(fatal_error.file, file);
     fatal_error.fd = fd;
     fatal_error.all_threads = all_threads;
@@ -733,12 +743,14 @@ faulthandler_dump_traceback_later(PyObject *self,
     if (!thread.running) {
         thread.running = PyThread_allocate_lock();
         if (!thread.running) {
+            Py_XDECREF(file);
             return PyErr_NoMemory();
         }
     }
     if (!thread.cancel_event) {
         thread.cancel_event = PyThread_allocate_lock();
         if (!thread.cancel_event || !thread.running) {
+            Py_XDECREF(file);
             return PyErr_NoMemory();
         }
 
@@ -750,14 +762,14 @@ faulthandler_dump_traceback_later(PyObject *self,
     /* format the timeout */
     header = format_timeout(timeout_us);
     if (header == NULL) {
+        Py_XDECREF(file);
         return PyErr_NoMemory();
     }
     header_len = strlen(header);
 
     /* Cancel previous thread, if running */
     cancel_dump_traceback_later();
 
-    Py_XINCREF(file);
     Py_XSETREF(thread.file, file);
     thread.fd = fd;
     /* the downcast is safe: we check that 0 < timeout_us < PY_TIMEOUT_MAX */
@@ -919,28 +931,31 @@ faulthandler_register_py(PyObject *self,
 
     if (user_signals == NULL) {
         user_signals = PyMem_Calloc(Py_NSIG, sizeof(user_signal_t));
-        if (user_signals == NULL)
+        if (user_signals == NULL) {
+            Py_XDECREF(file);
             return PyErr_NoMemory();
+        }
     }
     user = &user_signals[signum];
 
     if (!user->enabled) {
 #ifdef FAULTHANDLER_USE_ALT_STACK
         if (faulthandler_allocate_stack() < 0) {
+            Py_XDECREF(file);
             return NULL;
         }
 #endif
 
         err = faulthandler_register(signum, chain, &previous);
         if (err) {
             PyErr_SetFromErrno(PyExc_OSError);
+            Py_XDECREF(file);
             return NULL;
         }
 
         user->previous = previous;
     }
 
-    Py_XINCREF(file);
     Py_XSETREF(user->file, file);
     user->fd = fd;
     user->all_threads = all_threads;
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		Fix possible crashes in a free-threaded debug build related to concurrent
		change and use of the :mod:`sys` module attributes.