gh-126316: Make grp.getgrall() thread-safe: add a mutex (#127055)

grpmodule.c is no longer built with the limited C API, since PyMutex is excluded from the limited C API.
python · Nov 21, 2024 · 3c2bd66 · 3c2bd66
1 parent 60ec854
commit 3c2bd66
Show file tree

Hide file tree

Showing 8 changed files with 107 additions and 22 deletions.
diff --git a/Include/internal/pycore_global_objects_fini_generated.h b/Include/internal/pycore_global_objects_fini_generated.h
diff --git a/Include/internal/pycore_global_strings.h b/Include/internal/pycore_global_strings.h
@@ -471,6 +471,7 @@ struct _Py_global_strings {
         STRUCT_FOR_ID(hi)
         STRUCT_FOR_ID(hook)
         STRUCT_FOR_ID(hour)
+        STRUCT_FOR_ID(id)
         STRUCT_FOR_ID(ident)
         STRUCT_FOR_ID(identity_hint)
         STRUCT_FOR_ID(ignore)

diff --git a/Include/internal/pycore_runtime_init_generated.h b/Include/internal/pycore_runtime_init_generated.h
diff --git a/Include/internal/pycore_unicodeobject_generated.h b/Include/internal/pycore_unicodeobject_generated.h
diff --git a/Misc/NEWS.d/next/Library/2024-11-20-11-37-08.gh-issue-126316.ElkZmE.rst b/Misc/NEWS.d/next/Library/2024-11-20-11-37-08.gh-issue-126316.ElkZmE.rst
@@ -0,0 +1,2 @@
+:mod:`grp`: Make :func:`grp.getgrall` thread-safe by adding a mutex. Patch
+by Victor Stinner.
diff --git a/Modules/clinic/grpmodule.c.h b/Modules/clinic/grpmodule.c.h
diff --git a/Modules/grpmodule.c b/Modules/grpmodule.c
@@ -1,9 +1,8 @@
 /* UNIX group file access module */
 
-// Need limited C API version 3.13 for PyMem_RawRealloc()
-#include "pyconfig.h"   // Py_GIL_DISABLED
-#ifndef Py_GIL_DISABLED
-#define Py_LIMITED_API 0x030d0000
+// Argument Clinic uses the internal C API
+#ifndef Py_BUILD_CORE_BUILTIN
+#  define Py_BUILD_CORE_MODULE 1
 #endif
 
 #include "Python.h"
@@ -281,23 +280,33 @@ static PyObject *
 grp_getgrall_impl(PyObject *module)
 /*[clinic end generated code: output=585dad35e2e763d7 input=d7df76c825c367df]*/
 {
-    PyObject *d;
-    struct group *p;
-
-    if ((d = PyList_New(0)) == NULL)
+    PyObject *d = PyList_New(0);
+    if (d == NULL) {
         return NULL;
+    }
+
+    static PyMutex getgrall_mutex = {0};
+    PyMutex_Lock(&getgrall_mutex);
     setgrent();
+
+    struct group *p;
     while ((p = getgrent()) != NULL) {
+        // gh-126316: Don't release the mutex around mkgrent() since
+        // setgrent()/endgrent() are not reentrant / thread-safe. A deadlock
+        // is unlikely since mkgrent() should not be able to call arbitrary
+        // Python code.
         PyObject *v = mkgrent(module, p);
         if (v == NULL || PyList_Append(d, v) != 0) {
             Py_XDECREF(v);
-            Py_DECREF(d);
-            endgrent();
-            return NULL;
+            Py_CLEAR(d);
+            goto done;
         }
         Py_DECREF(v);
     }
+
+done:
     endgrent();
+    PyMutex_Unlock(&getgrall_mutex);
     return d;
 }
 

diff --git a/Tools/c-analyzer/cpython/ignored.tsv b/Tools/c-analyzer/cpython/ignored.tsv
@@ -739,6 +739,7 @@ Modules/expat/xmlrole.c	-	declClose	-
 Modules/expat/xmlrole.c	-	error	-
 
 ## other
+Modules/grpmodule.c	grp_getgrall_impl	getgrall_mutex	-
 Modules/_io/_iomodule.c	-	_PyIO_Module	-
 Modules/_sqlite/module.c	-	_sqlite3module	-
 Modules/clinic/md5module.c.h	_md5_md5	_keywords	-
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		:mod:`grp`: Make :func:`grp.getgrall` thread-safe by adding a mutex. Patch
		by Victor Stinner.