From 3c2bd66e21bd8de69a89ebf09ff9d8e78ddfb839 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 21 Nov 2024 15:47:24 +0100 Subject: [PATCH] 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. --- .../pycore_global_objects_fini_generated.h | 1 + Include/internal/pycore_global_strings.h | 1 + .../internal/pycore_runtime_init_generated.h | 1 + .../internal/pycore_unicodeobject_generated.h | 4 + ...-11-20-11-37-08.gh-issue-126316.ElkZmE.rst | 2 + Modules/clinic/grpmodule.c.h | 88 ++++++++++++++++--- Modules/grpmodule.c | 31 ++++--- Tools/c-analyzer/cpython/ignored.tsv | 1 + 8 files changed, 107 insertions(+), 22 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2024-11-20-11-37-08.gh-issue-126316.ElkZmE.rst diff --git a/Include/internal/pycore_global_objects_fini_generated.h b/Include/internal/pycore_global_objects_fini_generated.h index e4f0138e17e..c12e242d560 100644 --- a/Include/internal/pycore_global_objects_fini_generated.h +++ b/Include/internal/pycore_global_objects_fini_generated.h @@ -982,6 +982,7 @@ _PyStaticObjects_CheckRefcnt(PyInterpreterState *interp) { _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(hi)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(hook)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(hour)); + _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(id)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(ident)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(identity_hint)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(ignore)); diff --git a/Include/internal/pycore_global_strings.h b/Include/internal/pycore_global_strings.h index e70f11e2a26..dfd9f2b799e 100644 --- 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 index 5d404c8fd91..b631382cae0 100644 --- a/Include/internal/pycore_runtime_init_generated.h +++ b/Include/internal/pycore_runtime_init_generated.h @@ -980,6 +980,7 @@ extern "C" { INIT_ID(hi), \ INIT_ID(hook), \ INIT_ID(hour), \ + INIT_ID(id), \ INIT_ID(ident), \ INIT_ID(identity_hint), \ INIT_ID(ignore), \ diff --git a/Include/internal/pycore_unicodeobject_generated.h b/Include/internal/pycore_unicodeobject_generated.h index d0bc8d7186c..24cec3a4fde 100644 --- a/Include/internal/pycore_unicodeobject_generated.h +++ b/Include/internal/pycore_unicodeobject_generated.h @@ -1680,6 +1680,10 @@ _PyUnicode_InitStaticStrings(PyInterpreterState *interp) { _PyUnicode_InternStatic(interp, &string); assert(_PyUnicode_CheckConsistency(string, 1)); assert(PyUnicode_GET_LENGTH(string) != 1); + string = &_Py_ID(id); + _PyUnicode_InternStatic(interp, &string); + assert(_PyUnicode_CheckConsistency(string, 1)); + assert(PyUnicode_GET_LENGTH(string) != 1); string = &_Py_ID(ident); _PyUnicode_InternStatic(interp, &string); assert(_PyUnicode_CheckConsistency(string, 1)); 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 new file mode 100644 index 00000000000..d643254c5b3 --- /dev/null +++ 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 index cc0ad210f42..facfa3a43e4 100644 --- a/Modules/clinic/grpmodule.c.h +++ b/Modules/clinic/grpmodule.c.h @@ -2,6 +2,12 @@ preserve [clinic start generated code]*/ +#if defined(Py_BUILD_CORE) && !defined(Py_BUILD_CORE_MODULE) +# include "pycore_gc.h" // PyGC_Head +# include "pycore_runtime.h" // _Py_ID() +#endif +#include "pycore_modsupport.h" // _PyArg_UnpackKeywords() + PyDoc_STRVAR(grp_getgrgid__doc__, "getgrgid($module, /, id)\n" "--\n" @@ -11,21 +17,49 @@ PyDoc_STRVAR(grp_getgrgid__doc__, "If id is not valid, raise KeyError."); #define GRP_GETGRGID_METHODDEF \ - {"getgrgid", (PyCFunction)(void(*)(void))grp_getgrgid, METH_VARARGS|METH_KEYWORDS, grp_getgrgid__doc__}, + {"getgrgid", _PyCFunction_CAST(grp_getgrgid), METH_FASTCALL|METH_KEYWORDS, grp_getgrgid__doc__}, static PyObject * grp_getgrgid_impl(PyObject *module, PyObject *id); static PyObject * -grp_getgrgid(PyObject *module, PyObject *args, PyObject *kwargs) +grp_getgrgid(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames) { PyObject *return_value = NULL; - static char *_keywords[] = {"id", NULL}; + #if defined(Py_BUILD_CORE) && !defined(Py_BUILD_CORE_MODULE) + + #define NUM_KEYWORDS 1 + static struct { + PyGC_Head _this_is_not_used; + PyObject_VAR_HEAD + PyObject *ob_item[NUM_KEYWORDS]; + } _kwtuple = { + .ob_base = PyVarObject_HEAD_INIT(&PyTuple_Type, NUM_KEYWORDS) + .ob_item = { &_Py_ID(id), }, + }; + #undef NUM_KEYWORDS + #define KWTUPLE (&_kwtuple.ob_base.ob_base) + + #else // !Py_BUILD_CORE + # define KWTUPLE NULL + #endif // !Py_BUILD_CORE + + static const char * const _keywords[] = {"id", NULL}; + static _PyArg_Parser _parser = { + .keywords = _keywords, + .fname = "getgrgid", + .kwtuple = KWTUPLE, + }; + #undef KWTUPLE + PyObject *argsbuf[1]; PyObject *id; - if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O:getgrgid", _keywords, - &id)) + args = _PyArg_UnpackKeywords(args, nargs, NULL, kwnames, &_parser, + /*minpos*/ 1, /*maxpos*/ 1, /*minkw*/ 0, /*varpos*/ 0, argsbuf); + if (!args) { goto exit; + } + id = args[0]; return_value = grp_getgrgid_impl(module, id); exit: @@ -41,21 +75,53 @@ PyDoc_STRVAR(grp_getgrnam__doc__, "If name is not valid, raise KeyError."); #define GRP_GETGRNAM_METHODDEF \ - {"getgrnam", (PyCFunction)(void(*)(void))grp_getgrnam, METH_VARARGS|METH_KEYWORDS, grp_getgrnam__doc__}, + {"getgrnam", _PyCFunction_CAST(grp_getgrnam), METH_FASTCALL|METH_KEYWORDS, grp_getgrnam__doc__}, static PyObject * grp_getgrnam_impl(PyObject *module, PyObject *name); static PyObject * -grp_getgrnam(PyObject *module, PyObject *args, PyObject *kwargs) +grp_getgrnam(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames) { PyObject *return_value = NULL; - static char *_keywords[] = {"name", NULL}; + #if defined(Py_BUILD_CORE) && !defined(Py_BUILD_CORE_MODULE) + + #define NUM_KEYWORDS 1 + static struct { + PyGC_Head _this_is_not_used; + PyObject_VAR_HEAD + PyObject *ob_item[NUM_KEYWORDS]; + } _kwtuple = { + .ob_base = PyVarObject_HEAD_INIT(&PyTuple_Type, NUM_KEYWORDS) + .ob_item = { &_Py_ID(name), }, + }; + #undef NUM_KEYWORDS + #define KWTUPLE (&_kwtuple.ob_base.ob_base) + + #else // !Py_BUILD_CORE + # define KWTUPLE NULL + #endif // !Py_BUILD_CORE + + static const char * const _keywords[] = {"name", NULL}; + static _PyArg_Parser _parser = { + .keywords = _keywords, + .fname = "getgrnam", + .kwtuple = KWTUPLE, + }; + #undef KWTUPLE + PyObject *argsbuf[1]; PyObject *name; - if (!PyArg_ParseTupleAndKeywords(args, kwargs, "U:getgrnam", _keywords, - &name)) + args = _PyArg_UnpackKeywords(args, nargs, NULL, kwnames, &_parser, + /*minpos*/ 1, /*maxpos*/ 1, /*minkw*/ 0, /*varpos*/ 0, argsbuf); + if (!args) { goto exit; + } + if (!PyUnicode_Check(args[0])) { + _PyArg_BadArgument("getgrnam", "argument 'name'", "str", args[0]); + goto exit; + } + name = args[0]; return_value = grp_getgrnam_impl(module, name); exit: @@ -82,4 +148,4 @@ grp_getgrall(PyObject *module, PyObject *Py_UNUSED(ignored)) { return grp_getgrall_impl(module); } -/*[clinic end generated code: output=81f180beb67fc585 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=2154194308dab038 input=a9049054013a1b77]*/ diff --git a/Modules/grpmodule.c b/Modules/grpmodule.c index f7d3e12f347..29da9936b65 100644 --- 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 index 686f3935d91..4327a111eed 100644 --- 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 -