From fd957c124c44441d9c5eaf61f7af8cf266bafcb1 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 3 Nov 2020 18:07:15 +0100 Subject: [PATCH] bpo-41796: Call _PyAST_Fini() earlier to fix a leak (GH-23131) Call _PyAST_Fini() on all interpreters, not only on the main interpreter. Also, call it ealier to fix a reference leak. Python types contain a reference to themselves in in their PyTypeObject.tp_mro member. _PyAST_Fini() must called before the last GC collection to destroy AST types. _PyInterpreterState_Clear() now calls _PyAST_Fini(). It now also calls _PyWarnings_Fini() on subinterpeters, not only on the main interpreter. Add an assertion in AST init_types() to ensure that the _ast module is no longer used after _PyAST_Fini() has been called. --- Include/internal/pycore_pylifecycle.h | 2 +- Parser/asdl_c.py | 67 ++++++++++++++++++--------- Python/Python-ast.c | 33 ++++++++++--- Python/pylifecycle.c | 8 ---- Python/pystate.c | 11 +++-- 5 files changed, 78 insertions(+), 43 deletions(-) diff --git a/Include/internal/pycore_pylifecycle.h b/Include/internal/pycore_pylifecycle.h index 6d84e37232b..cba3bbdc2b2 100644 --- a/Include/internal/pycore_pylifecycle.h +++ b/Include/internal/pycore_pylifecycle.h @@ -84,7 +84,7 @@ extern void _PyFaulthandler_Fini(void); extern void _PyHash_Fini(void); extern void _PyTraceMalloc_Fini(void); extern void _PyWarnings_Fini(PyInterpreterState *interp); -extern void _PyAST_Fini(PyThreadState *tstate); +extern void _PyAST_Fini(PyInterpreterState *interp); extern PyStatus _PyGILState_Init(PyThreadState *tstate); extern void _PyGILState_Fini(PyThreadState *tstate); diff --git a/Parser/asdl_c.py b/Parser/asdl_c.py index 9a833e841de..9fec7ae017c 100755 --- a/Parser/asdl_c.py +++ b/Parser/asdl_c.py @@ -1015,18 +1015,35 @@ static int add_ast_fields(struct ast_state *state) """, 0, reflow=False) - self.emit("static int init_types(struct ast_state *state)",0) - self.emit("{", 0) - self.emit("if (state->initialized) return 1;", 1) - self.emit("if (init_identifiers(state) < 0) return 0;", 1) - self.emit("state->AST_type = PyType_FromSpec(&AST_type_spec);", 1) - self.emit("if (!state->AST_type) return 0;", 1) - self.emit("if (add_ast_fields(state) < 0) return 0;", 1) + self.file.write(textwrap.dedent(''' + static int + init_types(struct ast_state *state) + { + // init_types() must not be called after _PyAST_Fini() + // has been called + assert(state->initialized >= 0); + + if (state->initialized) { + return 1; + } + if (init_identifiers(state) < 0) { + return 0; + } + state->AST_type = PyType_FromSpec(&AST_type_spec); + if (!state->AST_type) { + return 0; + } + if (add_ast_fields(state) < 0) { + return 0; + } + ''')) for dfn in mod.dfns: self.visit(dfn) - self.emit("state->initialized = 1;", 1) - self.emit("return 1;", 1); - self.emit("}", 0) + self.file.write(textwrap.dedent(''' + state->initialized = 1; + return 1; + } + ''')) def visitProduct(self, prod, name): if prod.fields: @@ -1353,23 +1370,27 @@ def generate_ast_state(module_state, f): def generate_ast_fini(module_state, f): - f.write(""" -void _PyAST_Fini(PyThreadState *tstate) -{ -#ifdef Py_BUILD_CORE - struct ast_state *state = &tstate->interp->ast; -#else - struct ast_state *state = &global_ast_state; -#endif + f.write(textwrap.dedent(""" + void _PyAST_Fini(PyInterpreterState *interp) + { + #ifdef Py_BUILD_CORE + struct ast_state *state = &interp->ast; + #else + struct ast_state *state = &global_ast_state; + #endif -""") + """)) for s in module_state: f.write(" Py_CLEAR(state->" + s + ');\n') - f.write(""" - state->initialized = 0; -} + f.write(textwrap.dedent(""" + #if defined(Py_BUILD_CORE) && !defined(NDEBUG) + state->initialized = -1; + #else + state->initialized = 0; + #endif + } -""") + """)) def generate_module_def(mod, f, internal_h): diff --git a/Python/Python-ast.c b/Python/Python-ast.c index f04addbe201..a456b519514 100644 --- a/Python/Python-ast.c +++ b/Python/Python-ast.c @@ -261,10 +261,10 @@ get_ast_state(void) #include "Python-ast.h" #include "structmember.h" -void _PyAST_Fini(PyThreadState *tstate) +void _PyAST_Fini(PyInterpreterState *interp) { #ifdef Py_BUILD_CORE - struct ast_state *state = &tstate->interp->ast; + struct ast_state *state = &interp->ast; #else struct ast_state *state = &global_ast_state; #endif @@ -483,7 +483,11 @@ void _PyAST_Fini(PyThreadState *tstate) Py_CLEAR(state->vararg); Py_CLEAR(state->withitem_type); +#if defined(Py_BUILD_CORE) && !defined(NDEBUG) + state->initialized = -1; +#else state->initialized = 0; +#endif } static int init_identifiers(struct ast_state *state) @@ -1227,13 +1231,27 @@ static int add_ast_fields(struct ast_state *state) } -static int init_types(struct ast_state *state) + +static int +init_types(struct ast_state *state) { - if (state->initialized) return 1; - if (init_identifiers(state) < 0) return 0; + // init_types() must not be called after _PyAST_Fini() + // has been called + assert(state->initialized >= 0); + + if (state->initialized) { + return 1; + } + if (init_identifiers(state) < 0) { + return 0; + } state->AST_type = PyType_FromSpec(&AST_type_spec); - if (!state->AST_type) return 0; - if (add_ast_fields(state) < 0) return 0; + if (!state->AST_type) { + return 0; + } + if (add_ast_fields(state) < 0) { + return 0; + } state->mod_type = make_type(state, "mod", state->AST_type, NULL, 0, "mod = Module(stmt* body, type_ignore* type_ignores)\n" " | Interactive(stmt* body)\n" @@ -1902,6 +1920,7 @@ static int init_types(struct ast_state *state) TypeIgnore_fields, 2, "TypeIgnore(int lineno, string tag)"); if (!state->TypeIgnore_type) return 0; + state->initialized = 1; return 1; } diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index ff58c1b9153..cad0fa7026b 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1545,12 +1545,6 @@ flush_std_files(void) static void finalize_interp_types(PyThreadState *tstate) { - // The _ast module state is shared by all interpreters. - // The state must only be cleared by the main interpreter. - if (_Py_IsMainInterpreter(tstate)) { - _PyAST_Fini(tstate); - } - _PyExc_Fini(tstate); _PyFrame_Fini(tstate); _PyAsyncGen_Fini(tstate); @@ -1591,8 +1585,6 @@ finalize_interp_clear(PyThreadState *tstate) _Py_ClearFileSystemEncoding(); } - _PyWarnings_Fini(tstate->interp); - finalize_interp_types(tstate); } diff --git a/Python/pystate.c b/Python/pystate.c index e37cbd5a657..c9882a7f74b 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -300,13 +300,16 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate) Py_CLEAR(interp->after_forkers_parent); Py_CLEAR(interp->after_forkers_child); #endif - if (_PyRuntimeState_GetFinalizing(runtime) == NULL) { - _PyWarnings_Fini(interp); - } + + _PyAST_Fini(interp); + _PyWarnings_Fini(interp); + + // All Python types must be destroyed before the last GC collection. Python + // types create a reference cycle to themselves in their in their + // PyTypeObject.tp_mro member (the tuple contains the type). /* Last garbage collection on this interpreter */ _PyGC_CollectNoFail(tstate); - _PyGC_Fini(tstate); /* We don't clear sysdict and builtins until the end of this function.