From: Eric Snow Date: Wed, 22 May 2024 22:26:58 +0000 (-0400) Subject: [3.12] gh-119213: Be More Careful About _PyArg_Parser.kwtuple Across Interpreters... X-Git-Tag: v3.12.4~69 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=0d5fe2c7b417856ec62e46339aa769bce8dd5f12;p=thirdparty%2FPython%2Fcpython.git [3.12] gh-119213: Be More Careful About _PyArg_Parser.kwtuple Across Interpreters (gh-119331) (gh-119425) _PyArg_Parser holds static global data generated for modules by Argument Clinic. The _PyArg_Parser.kwtuple field is a tuple object, even though it's stored within a static global. In some cases the tuple is statically allocated and thus it's okay that it gets shared by multiple interpreters. However, in other cases the tuple is set lazily, allocated from the heap using the active interprepreter at the point the tuple is needed. This is a problem once that interpreter is destroyed since _PyArg_Parser.kwtuple becomes at dangling pointer, leading to crashes. It isn't a problem if the tuple is allocated under the main interpreter, since its lifetime is bound to the lifetime of the runtime. The solution here is to temporarily switch to the main interpreter. The alternative would be to always statically allocate the tuple. This change also fixes a bug where only the most recent parser was added to the global linked list. (cherry picked from commit 81865002aee8eaaeb3c7e402f86183afa6de77bf) --- diff --git a/Doc/data/python3.12.abi b/Doc/data/python3.12.abi index ab5190f6966c..93dc9dbc7117 100644 --- a/Doc/data/python3.12.abi +++ b/Doc/data/python3.12.abi @@ -1710,7 +1710,7 @@ - + @@ -2072,7 +2072,7 @@ - + @@ -2524,7 +2524,7 @@ - + @@ -2548,39 +2548,39 @@ - + - + - + - + - - + + - - + + - - - + + + - - - + + + - - - + + + @@ -3679,7 +3679,7 @@ - + @@ -3698,7 +3698,7 @@ - + @@ -3724,65 +3724,68 @@ - - + + - + - - - - + + + + - + - - + - - + + - + + + + + - - + + - + - + - - + + - - - - + + + + - - - + + + - - + + - - + + @@ -3896,22 +3899,22 @@ - - - + + + - - + + - - + + - - - + + + @@ -4033,7 +4036,7 @@ - + @@ -4089,22 +4092,22 @@ - + - + - + - + @@ -4750,7 +4753,7 @@ - + @@ -4908,27 +4911,27 @@ - + - + - + - + - + @@ -5189,7 +5192,7 @@ - + @@ -5374,7 +5377,7 @@ - + @@ -5411,12 +5414,12 @@ - + - + @@ -5640,7 +5643,7 @@ - + @@ -5672,21 +5675,21 @@ - + - + - + - + @@ -5820,12 +5823,12 @@ - + - + @@ -6136,7 +6139,7 @@ - + @@ -6169,11 +6172,11 @@ - + - + @@ -6590,11 +6593,11 @@ - + - + @@ -6875,9 +6878,9 @@ - - - + + + @@ -7125,41 +7128,41 @@ - - + + - - - - + + + + - - - - + + + + - - - - + + + + - - - + + + - - - + + + - - - + + + @@ -7305,9 +7308,9 @@ - - - + + + @@ -7414,7 +7417,7 @@ - + @@ -7460,7 +7463,7 @@ - + @@ -7484,23 +7487,27 @@ - + - + - + - + + + + + - + @@ -7537,30 +7544,30 @@ - - + + - - - - + + + + - - + + - - - - + + + + - - - - + + + + @@ -7582,15 +7589,15 @@ - + - + - + @@ -7648,15 +7655,15 @@ - + - + - + @@ -7717,7 +7724,7 @@ - + @@ -7807,42 +7814,42 @@ - - - - - + + + + + - - - + + + - - - + + + - - - + + + - - - + + + - + - - + + - - + + @@ -7925,33 +7932,33 @@ - + - + - + - + - + - + - + - + - + - + @@ -7993,7 +8000,7 @@ - + @@ -8154,7 +8161,7 @@ - + @@ -8217,12 +8224,18 @@ + + + + + + - - + + @@ -8426,13 +8439,13 @@ - + - + - + @@ -8616,21 +8629,21 @@ - - + + - - + + - - - + + + - - + + @@ -8663,40 +8676,40 @@ - + - + - + - - + + - + - + - + - + - + - + - + @@ -8837,16 +8850,16 @@ - + - - - + + + @@ -9062,7 +9075,7 @@ - + @@ -9663,50 +9676,50 @@ - - - + + + - - - + + + - - - - + + + + - - - + + + - - - - - + + + + + - - - - + + + + - - - + + + - - + + - + @@ -10841,7 +10854,7 @@ - + @@ -10912,10 +10925,10 @@ - + - + @@ -11147,7 +11160,7 @@ - + @@ -11420,7 +11433,7 @@ - + @@ -11428,8 +11441,8 @@ - - + + @@ -11948,7 +11961,7 @@ - + @@ -11974,28 +11987,28 @@ - - - - - + + + + + - + - + - + - + - + - + @@ -12064,10 +12077,10 @@ - + - + @@ -13232,42 +13245,42 @@ - + - + - + - + - + - + - + - + - + - + - + - + - + @@ -13392,12 +13405,12 @@ - + - + @@ -13410,19 +13423,19 @@ - + - + - + - + - + @@ -13474,7 +13487,7 @@ - + @@ -13488,18 +13501,18 @@ - + - + - - + + - - + + @@ -13528,7 +13541,7 @@ - + @@ -13543,7 +13556,7 @@ - + @@ -13564,13 +13577,13 @@ - + - + - + @@ -13606,7 +13619,7 @@ - + @@ -13678,7 +13691,7 @@ - + @@ -13707,7 +13720,7 @@ - + @@ -14307,7 +14320,7 @@ - + @@ -14367,7 +14380,7 @@ - + @@ -14421,7 +14434,7 @@ - + @@ -14580,7 +14593,7 @@ - + @@ -14664,7 +14677,7 @@ - + @@ -15147,7 +15160,7 @@ - + @@ -15282,7 +15295,7 @@ - + @@ -15366,7 +15379,7 @@ - + @@ -15438,7 +15451,7 @@ - + @@ -15534,223 +15547,226 @@ - + - + - - + + - - + + - + - - + + - + - + - + - + - + - + - - + + - - + + - - + + - + - - + + - + - + - - + + - + - + - + - - + + - + - - + + - - + + - - + + - + - + - - + + - + - + - - + + - + - + - + - - + + - + - - + + - - + + - - + + - - + + - + - - + + - + - + - + - - + + - + - + - + - + - - + + - + - - + + - + - + - + - + - - + + - - + + - + - + - + - + - + - - + + - - + + - + - - + + - + - + + + + @@ -15873,20 +15889,20 @@ - + - + - + - + - + - + @@ -16043,7 +16059,7 @@ - + @@ -16054,7 +16070,7 @@ - + @@ -16497,7 +16513,7 @@ - + @@ -16613,7 +16629,7 @@ - + @@ -16722,16 +16738,16 @@ - + - + - + - + @@ -17056,7 +17072,7 @@ - + @@ -17802,13 +17818,13 @@ - + - + @@ -18653,76 +18669,76 @@ - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + @@ -18731,86 +18747,86 @@ - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + @@ -18819,11 +18835,11 @@ - + - + @@ -18837,40 +18853,40 @@ - + - + - + - + - + - + - + @@ -18881,22 +18897,22 @@ - + - + - + - + @@ -18906,39 +18922,39 @@ - + - + - + - + - + - + - + @@ -18949,31 +18965,31 @@ - + - + - + - + - + - + @@ -19016,7 +19032,7 @@ - + @@ -19028,7 +19044,7 @@ - + @@ -20032,41 +20048,41 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + - + @@ -20762,25 +20778,25 @@ - + - + - + - + @@ -20879,19 +20895,19 @@ - + - + - + - + @@ -20899,7 +20915,7 @@ - + @@ -20910,12 +20926,12 @@ - + - + @@ -21077,7 +21093,7 @@ - + @@ -21267,7 +21283,7 @@ - + @@ -21360,7 +21376,7 @@ - + @@ -21376,6 +21392,11 @@ + + + + + @@ -21673,11 +21694,11 @@ - + - + @@ -21704,11 +21725,11 @@ - + - + @@ -21789,7 +21810,7 @@ - + @@ -21814,21 +21835,21 @@ - + - + - + - + @@ -21944,7 +21965,7 @@ - + @@ -22010,30 +22031,30 @@ - - + + - + - - + + - - + + - - + + - - + + - + @@ -22072,7 +22093,7 @@ - + @@ -22301,8 +22322,8 @@ - - + + @@ -22460,11 +22481,11 @@ - + - + @@ -22816,41 +22837,41 @@ - - - - + + + + - - - - - - + + + + + + - - - - + + + + - + - - - - + + + + - - - - - + + + + + @@ -22964,7 +22985,7 @@ - + @@ -23357,12 +23378,12 @@ - - + + - - + + @@ -23431,43 +23452,43 @@ - + - + - + - + - + - + - + - + - + - + - + - + - + @@ -23544,107 +23565,123 @@ - - - + + + + + + + + + + + + + + + + + + + - - - + + + - - - - + + + + - - - - + + + + - - - - + + + + - - - - + + + + - - - - - - + + + + + + - - - - - - + + + + + + - - - - + + + + - - - - + + + + - - - - - + + + + + - - - - - + + + + + - - - - - + + + + + - - - - - + + + + + - - - + + + @@ -24103,6 +24140,10 @@ + + + + @@ -24134,121 +24175,121 @@ - + - - + + - - + + - - + + - - + + - - + + - - + + - - - + + + - - - - + + + + - - - + + + - - - + + + - - + + - - - + + + - + - + - - - + + + - - + + - - + + - - - - + + + + - - - - + + + + - - + + - - - + + + - - - - + + + + - - - - - + + + + + - - + + - + @@ -24275,7 +24316,7 @@ - + @@ -24581,7 +24622,7 @@ - + @@ -24759,21 +24800,21 @@ - + - + - + - + - + - + @@ -24809,21 +24850,24 @@ - + + + + - + - + - + @@ -25259,7 +25303,7 @@ - + @@ -25273,18 +25317,6 @@ - - - - - - - - - - - - @@ -25307,9 +25339,6 @@ - - - @@ -25461,7 +25490,7 @@ - + @@ -25481,16 +25510,16 @@ - + - + - + @@ -25558,10 +25587,6 @@ - - - - @@ -25818,128 +25843,128 @@ - - - - - + + + + + - - - - + + + + - - - + + + - - - - - - - - + + + + + + + + - - - - - - + + + + + + - - - + + + - - - - + + + + - - - - + + + + - - - - - - + + + + + + - - - - - - - + + + + + + + - - - - - - - + + + + + + + - - - + + + - - - - + + + + - - - - - + + + + + - - + + - - - - + + + + - - - - - + + + + + - - - + + + - - - + + + @@ -26220,12 +26245,12 @@ - + - + @@ -26421,38 +26446,38 @@ - - - + + + - - - - + + + + - + - - + + - - + + - - - + + + - - + + - + @@ -26469,7 +26494,7 @@ - + diff --git a/Include/internal/pycore_global_objects_fini_generated.h b/Include/internal/pycore_global_objects_fini_generated.h index 439f47a263df..909fe90b3f56 100644 --- a/Include/internal/pycore_global_objects_fini_generated.h +++ b/Include/internal/pycore_global_objects_fini_generated.h @@ -1186,6 +1186,7 @@ _PyStaticObjects_CheckRefcnt(PyInterpreterState *interp) { _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(sound)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(source)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(source_traceback)); + _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(spam)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(src)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(src_dir_fd)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(stacklevel)); diff --git a/Include/internal/pycore_global_strings.h b/Include/internal/pycore_global_strings.h index 0c84999cbf81..26c1be3b2aa9 100644 --- a/Include/internal/pycore_global_strings.h +++ b/Include/internal/pycore_global_strings.h @@ -675,6 +675,7 @@ struct _Py_global_strings { STRUCT_FOR_ID(sound) STRUCT_FOR_ID(source) STRUCT_FOR_ID(source_traceback) + STRUCT_FOR_ID(spam) STRUCT_FOR_ID(src) STRUCT_FOR_ID(src_dir_fd) STRUCT_FOR_ID(stacklevel) diff --git a/Include/internal/pycore_runtime_init_generated.h b/Include/internal/pycore_runtime_init_generated.h index 07f237b29058..dbfb633c7090 100644 --- a/Include/internal/pycore_runtime_init_generated.h +++ b/Include/internal/pycore_runtime_init_generated.h @@ -1181,6 +1181,7 @@ extern "C" { INIT_ID(sound), \ INIT_ID(source), \ INIT_ID(source_traceback), \ + INIT_ID(spam), \ INIT_ID(src), \ INIT_ID(src_dir_fd), \ INIT_ID(stacklevel), \ diff --git a/Include/internal/pycore_unicodeobject_generated.h b/Include/internal/pycore_unicodeobject_generated.h index 9b470094b7af..9f9e23f5cdeb 100644 --- a/Include/internal/pycore_unicodeobject_generated.h +++ b/Include/internal/pycore_unicodeobject_generated.h @@ -1866,6 +1866,9 @@ _PyUnicode_InitStaticStrings(PyInterpreterState *interp) { string = &_Py_ID(source_traceback); assert(_PyUnicode_CheckConsistency(string, 1)); _PyUnicode_InternInPlace(interp, &string); + string = &_Py_ID(spam); + assert(_PyUnicode_CheckConsistency(string, 1)); + _PyUnicode_InternInPlace(interp, &string); string = &_Py_ID(src); assert(_PyUnicode_CheckConsistency(string, 1)); _PyUnicode_InternInPlace(interp, &string); diff --git a/Lib/test/test_capi/test_getargs.py b/Lib/test/test_capi/test_getargs.py index 3d8768b8845f..69bd0f4c5f19 100644 --- a/Lib/test/test_capi/test_getargs.py +++ b/Lib/test/test_capi/test_getargs.py @@ -4,11 +4,17 @@ import string import sys from test import support from test.support import import_helper +from test.support import script_helper from test.support import warnings_helper # Skip this test if the _testcapi module isn't available. _testcapi = import_helper.import_module('_testcapi') from _testcapi import getargs_keywords, getargs_keyword_only +try: + import _testinternalcapi +except ImportError: + _testinternalcapi = NULL + # > How about the following counterproposal. This also changes some of # > the other format codes to be a little more regular. # > @@ -1369,6 +1375,33 @@ class Test_testcapi(unittest.TestCase): "argument 1 must be sequence of length 1, not 0"): parse(((),), {}, '(' + f + ')', ['a']) + @unittest.skipIf(_testinternalcapi is None, 'needs _testinternalcapi') + def test_gh_119213(self): + rc, out, err = script_helper.assert_python_ok("-c", """if True: + from test import support + script = '''if True: + import _testinternalcapi + _testinternalcapi.gh_119213_getargs(spam='eggs') + ''' + config = dict( + allow_fork=False, + allow_exec=False, + allow_threads=True, + allow_daemon_threads=False, + use_main_obmalloc=False, + gil=2, + check_multi_interp_extensions=True, + ) + rc = support.run_in_subinterp_with_config(script, **config) + assert rc == 0 + + # The crash is different if the interpreter was not destroyed first. + #interpid = _testinternalcapi.create_interpreter() + #rc = _testinternalcapi.exec_interpreter(interpid, script) + #assert rc == 0 + """) + self.assertEqual(rc, 0) + if __name__ == "__main__": unittest.main() diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-05-21-11-27-14.gh-issue-119213.nxjxrt.rst b/Misc/NEWS.d/next/Core and Builtins/2024-05-21-11-27-14.gh-issue-119213.nxjxrt.rst new file mode 100644 index 000000000000..e9073b4ba087 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-05-21-11-27-14.gh-issue-119213.nxjxrt.rst @@ -0,0 +1,3 @@ +Non-builtin modules built with argument clinic were crashing if used in a +subinterpreter before the main interpreter. The objects that were causing +the problem by leaking between interpreters carelessly have been fixed. diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c index 22d156725f54..c758420c3dea 100644 --- a/Modules/_testinternalcapi.c +++ b/Modules/_testinternalcapi.c @@ -1055,6 +1055,24 @@ pending_identify(PyObject *self, PyObject *args) } +/*[clinic input] +gh_119213_getargs + + spam: object = None + +Test _PyArg_Parser.kwtuple +[clinic start generated code]*/ + +static PyObject * +gh_119213_getargs_impl(PyObject *module, PyObject *spam) +/*[clinic end generated code: output=d8d9c95d5b446802 input=65ef47511da80fc2]*/ +{ + // It must never have been called in the main interprer + assert(!_Py_IsMainInterpreter(PyInterpreterState_Get())); + return Py_NewRef(spam); +} + + static PyMethodDef module_functions[] = { {"get_configs", get_configs, METH_NOARGS}, {"get_recursion_depth", get_recursion_depth, METH_NOARGS}, @@ -1087,6 +1105,7 @@ static PyMethodDef module_functions[] = { {"pending_threadfunc", _PyCFunction_CAST(pending_threadfunc), METH_VARARGS | METH_KEYWORDS}, {"pending_identify", pending_identify, METH_VARARGS, NULL}, + GH_119213_GETARGS_METHODDEF {NULL, NULL} /* sentinel */ }; diff --git a/Modules/clinic/_testinternalcapi.c.h b/Modules/clinic/_testinternalcapi.c.h index f51241258745..1cc4ad4c16c2 100644 --- a/Modules/clinic/_testinternalcapi.c.h +++ b/Modules/clinic/_testinternalcapi.c.h @@ -206,4 +206,64 @@ _testinternalcapi_assemble_code_object(PyObject *module, PyObject *const *args, exit: return return_value; } -/*[clinic end generated code: output=2965f1578b986218 input=a9049054013a1b77]*/ + +PyDoc_STRVAR(gh_119213_getargs__doc__, +"gh_119213_getargs($module, /, spam=None)\n" +"--\n" +"\n" +"Test _PyArg_Parser.kwtuple"); + +#define GH_119213_GETARGS_METHODDEF \ + {"gh_119213_getargs", _PyCFunction_CAST(gh_119213_getargs), METH_FASTCALL|METH_KEYWORDS, gh_119213_getargs__doc__}, + +static PyObject * +gh_119213_getargs_impl(PyObject *module, PyObject *spam); + +static PyObject * +gh_119213_getargs(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(spam), }, + }; + #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[] = {"spam", NULL}; + static _PyArg_Parser _parser = { + .keywords = _keywords, + .fname = "gh_119213_getargs", + .kwtuple = KWTUPLE, + }; + #undef KWTUPLE + PyObject *argsbuf[1]; + Py_ssize_t noptargs = nargs + (kwnames ? PyTuple_GET_SIZE(kwnames) : 0) - 0; + PyObject *spam = Py_None; + + args = _PyArg_UnpackKeywords(args, nargs, NULL, kwnames, &_parser, 0, 1, 0, argsbuf); + if (!args) { + goto exit; + } + if (!noptargs) { + goto skip_optional_pos; + } + spam = args[0]; +skip_optional_pos: + return_value = gh_119213_getargs_impl(module, spam); + +exit: + return return_value; +} +/*[clinic end generated code: output=1fa5cb831dbb391f input=a9049054013a1b77]*/ diff --git a/Python/getargs.c b/Python/getargs.c index 5e731cdc23cb..02bddf0618e5 100644 --- a/Python/getargs.c +++ b/Python/getargs.c @@ -4,6 +4,7 @@ #include "Python.h" #include "pycore_tuple.h" // _PyTuple_ITEMS() #include "pycore_pylifecycle.h" // _PyArg_Fini +#include "pycore_pystate.h" // _Py_IsMainInterpreter() #include #include @@ -2002,7 +2003,23 @@ _parser_init(struct _PyArg_Parser *parser) int owned; PyObject *kwtuple = parser->kwtuple; if (kwtuple == NULL) { + /* We may temporarily switch to the main interpreter to avoid + * creating a tuple that could outlive its owning interpreter. */ + PyThreadState *save_tstate = NULL; + PyThreadState *temp_tstate = NULL; + if (!_Py_IsMainInterpreter(PyInterpreterState_Get())) { + temp_tstate = PyThreadState_New(_PyInterpreterState_Main()); + if (temp_tstate == NULL) { + return -1; + } + save_tstate = PyThreadState_Swap(temp_tstate); + } kwtuple = new_kwtuple(keywords, len, pos); + if (temp_tstate != NULL) { + PyThreadState_Clear(temp_tstate); + (void)PyThreadState_Swap(save_tstate); + PyThreadState_Delete(temp_tstate); + } if (kwtuple == NULL) { return 0; }