From 0d5fe2c7b417856ec62e46339aa769bce8dd5f12 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 22 May 2024 18:26:58 -0400 Subject: [PATCH] [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) --- Doc/data/python3.12.abi | 1761 +++++++++-------- .../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 | 3 + Lib/test/test_capi/test_getargs.py | 33 + ...-05-21-11-27-14.gh-issue-119213.nxjxrt.rst | 3 + Modules/_testinternalcapi.c | 19 + Modules/clinic/_testinternalcapi.c.h | 62 +- Python/getargs.c | 17 + 10 files changed, 1032 insertions(+), 869 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-05-21-11-27-14.gh-issue-119213.nxjxrt.rst diff --git a/Doc/data/python3.12.abi b/Doc/data/python3.12.abi index ab5190f6966c02..93dc9dbc71174a 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 439f47a263dfa1..909fe90b3f56a0 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 0c84999cbf8127..26c1be3b2aa993 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 07f237b2905864..dbfb633c7090e8 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 9b470094b7afe2..9f9e23f5cdeb69 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 3d8768b8845f8d..69bd0f4c5f197c 100644 --- a/Lib/test/test_capi/test_getargs.py +++ b/Lib/test/test_capi/test_getargs.py @@ -4,11 +4,17 @@ 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 @@ def test_nested_tuple(self): "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 00000000000000..e9073b4ba08798 --- /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 22d156725f545c..c758420c3deab4 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 f5124125874503..1cc4ad4c16c2bb 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 5e731cdc23cb5f..02bddf0618e578 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; }