From c36d77dab36ee0f291e5117ae7ef26ac98782f2a Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 21 Nov 2024 15:47:24 +0100 Subject: [PATCH 1/2] 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. (cherry picked from commit 3c2bd66e21bd8de69a89ebf09ff9d8e78ddfb839) --- .../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 | 86 ++++++++++++++++--- Modules/grpmodule.c | 31 ++++--- Tools/c-analyzer/cpython/ignored.tsv | 1 + 8 files changed, 105 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 cd56ffde9e555e..65780e63a41d0b 100644 --- a/Include/internal/pycore_global_objects_fini_generated.h +++ b/Include/internal/pycore_global_objects_fini_generated.h @@ -978,6 +978,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 cad2d1a8d22049..b4721648ddd876 100644 --- a/Include/internal/pycore_global_strings.h +++ b/Include/internal/pycore_global_strings.h @@ -467,6 +467,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 19a6b9b1537d51..5a57eff1c24a26 100644 --- a/Include/internal/pycore_runtime_init_generated.h +++ b/Include/internal/pycore_runtime_init_generated.h @@ -976,6 +976,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 7f6b6e07984a9a..0b8f224fe4c8f2 100644 --- a/Include/internal/pycore_unicodeobject_generated.h +++ b/Include/internal/pycore_unicodeobject_generated.h @@ -1668,6 +1668,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 00000000000000..d643254c5b3564 --- /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 cc0ad210f42743..1da061f3fd8eb1 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,48 @@ 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, 1, 1, 0, argsbuf); + if (!args) { goto exit; + } + id = args[0]; return_value = grp_getgrgid_impl(module, id); exit: @@ -41,21 +74,52 @@ 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, 1, 1, 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 +146,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=2f7011384604d38d input=a9049054013a1b77]*/ diff --git a/Modules/grpmodule.c b/Modules/grpmodule.c index f7d3e12f347ec2..29da9936b65504 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 260eaa4f14f767..5e271a5014cb35 100644 --- a/Tools/c-analyzer/cpython/ignored.tsv +++ b/Tools/c-analyzer/cpython/ignored.tsv @@ -742,6 +742,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 - From d5a4a4c0d1d44a54b5c4895f4b678071446bb786 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 25 Nov 2024 15:20:17 +0100 Subject: [PATCH 2/2] Revert ABI changes Don't use Argument Clinic for grp.getgrgid() to avoid changing the ABI (change PyInterpreterState structure by adding an "id" identifier). --- .../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 -- Modules/clinic/grpmodule.c.h | 59 +------------------ Modules/grpmodule.c | 30 ++++++---- 6 files changed, 18 insertions(+), 78 deletions(-) diff --git a/Include/internal/pycore_global_objects_fini_generated.h b/Include/internal/pycore_global_objects_fini_generated.h index 65780e63a41d0b..cd56ffde9e555e 100644 --- a/Include/internal/pycore_global_objects_fini_generated.h +++ b/Include/internal/pycore_global_objects_fini_generated.h @@ -978,7 +978,6 @@ _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 b4721648ddd876..cad2d1a8d22049 100644 --- a/Include/internal/pycore_global_strings.h +++ b/Include/internal/pycore_global_strings.h @@ -467,7 +467,6 @@ 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 5a57eff1c24a26..19a6b9b1537d51 100644 --- a/Include/internal/pycore_runtime_init_generated.h +++ b/Include/internal/pycore_runtime_init_generated.h @@ -976,7 +976,6 @@ 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 0b8f224fe4c8f2..7f6b6e07984a9a 100644 --- a/Include/internal/pycore_unicodeobject_generated.h +++ b/Include/internal/pycore_unicodeobject_generated.h @@ -1668,10 +1668,6 @@ _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/Modules/clinic/grpmodule.c.h b/Modules/clinic/grpmodule.c.h index 1da061f3fd8eb1..43700a11c15c91 100644 --- a/Modules/clinic/grpmodule.c.h +++ b/Modules/clinic/grpmodule.c.h @@ -8,63 +8,6 @@ preserve #endif #include "pycore_modsupport.h" // _PyArg_UnpackKeywords() -PyDoc_STRVAR(grp_getgrgid__doc__, -"getgrgid($module, /, id)\n" -"--\n" -"\n" -"Return the group database entry for the given numeric group ID.\n" -"\n" -"If id is not valid, raise KeyError."); - -#define GRP_GETGRGID_METHODDEF \ - {"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 *const *args, Py_ssize_t nargs, PyObject *kwnames) -{ - PyObject *return_value = 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; - - args = _PyArg_UnpackKeywords(args, nargs, NULL, kwnames, &_parser, 1, 1, 0, argsbuf); - if (!args) { - goto exit; - } - id = args[0]; - return_value = grp_getgrgid_impl(module, id); - -exit: - return return_value; -} - PyDoc_STRVAR(grp_getgrnam__doc__, "getgrnam($module, /, name)\n" "--\n" @@ -146,4 +89,4 @@ grp_getgrall(PyObject *module, PyObject *Py_UNUSED(ignored)) { return grp_getgrall_impl(module); } -/*[clinic end generated code: output=2f7011384604d38d input=a9049054013a1b77]*/ +/*[clinic end generated code: output=1168333f1b15de11 input=a9049054013a1b77]*/ diff --git a/Modules/grpmodule.c b/Modules/grpmodule.c index 29da9936b65504..e0534576ff1e82 100644 --- a/Modules/grpmodule.c +++ b/Modules/grpmodule.c @@ -109,20 +109,15 @@ mkgrent(PyObject *module, struct group *p) return v; } -/*[clinic input] -grp.getgrgid - - id: object - -Return the group database entry for the given numeric group ID. - -If id is not valid, raise KeyError. -[clinic start generated code]*/ - static PyObject * -grp_getgrgid_impl(PyObject *module, PyObject *id) -/*[clinic end generated code: output=30797c289504a1ba input=15fa0e2ccf5cda25]*/ +grp_getgrgid(PyObject *module, PyObject *args, PyObject *kwargs) { + static char *kwlist[] = {"id", NULL}; + PyObject *id; + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O", kwlist, &id)) { + return NULL; + } + PyObject *retval = NULL; int nomem = 0; char *buf = NULL, *buf2 = NULL; @@ -189,6 +184,15 @@ grp_getgrgid_impl(PyObject *module, PyObject *id) return retval; } +PyDoc_STRVAR(grp_getgrgid__doc__, +"getgrgid($module, /, id)\n" +"--\n" +"\n" +"Return the group database entry for the given numeric group ID.\n" +"\n" +"If id is not valid, raise KeyError."); + + /*[clinic input] grp.getgrnam @@ -311,7 +315,7 @@ grp_getgrall_impl(PyObject *module) } static PyMethodDef grp_methods[] = { - GRP_GETGRGID_METHODDEF + {"getgrgid", _PyCFunction_CAST(grp_getgrgid), METH_VARARGS|METH_KEYWORDS, grp_getgrgid__doc__}, GRP_GETGRNAM_METHODDEF GRP_GETGRALL_METHODDEF {NULL, NULL}