8000 bpo-41631: _ast module uses again a global state (#21961) · python/cpython@e5fbe0c · GitHub
[go: up one dir, main page]
Skip to content

Commit e5fbe0c

Browse files
vstinnervstinner
authored
bpo-41631: _ast module uses again a global state (#21961)
Partially revert commit ac46eb4: "bpo-38113: Update the Python-ast.c generator to PEP384 (gh-15957)". Using a module state per module instance is causing subtle practical problems. For example, the Mercurial project replaces the __import__() function to implement lazy import, whereas Python expected that "import _ast" always return a fully initialized _ast module. Add _PyAST_Fini() to clear the state at exit. The _ast module has no state (set _astmodule.m_size to 0). Remove astmodule_traverse(), astmodule_clear() and astmodule_free() functions.
1 parent 7bcc645 commit e5fbe0c

File tree

7 files changed

+160
-309
lines changed

7 files changed

+160
-309
lines changed

‎Include/internal/pycore_pylifecycle.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ extern void _PyFaulthandler_Fini(void);
8484
extern void _PyHash_Fini(void);
8585
extern void _PyTraceMalloc_Fini(void);
8686
extern void _PyWarnings_Fini(PyInterpreterState *interp);
87+
extern void _PyAST_Fini(PyThreadState *tstate);
8788

8889
extern PyStatus _PyGILState_Init(PyThreadState *tstate);
8990
extern void _PyGILState_Fini(PyThreadState *tstate);

‎Lib/ast.py

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -497,18 +497,20 @@ def generic_visit(self, node):
497497
return node
498498

499499

500-
# The following code is for backward compatibility.
501-
# It will be removed in future.
500+
# If the ast module is loaded more than once, only add deprecated methods once
501+
if not hasattr(Constant, 'n'):
502+
# The following code is for backward compatibility.
503+
# It will be removed in future.
502504

503-
def _getter(self):
504-
"""Deprecated. Use value instead."""
505-
return self.value
505+
def _getter(self):
506+
"""Deprecated. Use value instead."""
507+
return self.value
506508

507-
def _setter(self, value):
508-
self.value = value
509+
def _setter(self, value):
510+
self.value = value
509511

510-
Constant.n = property(_getter, _setter)
511-
Constant.s = property(_getter, _setter)
512+
Constant.n = property(_getter, _setter)
513+
Constant.s = property(_getter, _setter)
512514

513515
class _ABC(type):
514516

@@ -600,14 +602,19 @@ class ExtSlice(slice):
600602
def __new__(cls, dims=(), **kwargs):
601603
return Tuple(list(dims), Load(), **kwargs)
602604

603-
def _dims_getter(self):
604-
"""Deprecated. Use elts instead."""
605-
return self.elts
605+
# If the ast module is loaded more than once, only add deprecated methods once
606+
if not hasattr(Tuple, 'dims'):
607+
# The following code is for backward compatibility.
608+
# It will be removed in future.
606609

607-
def _dims_setter(self, value):
608-
self.elts = value
610+
def _dims_getter(self):
611+
"""Deprecated. Use elts instead."""
612+
return self.elts
609613

610-
Tuple.dims = property(_dims_getter, _dims_setter)
614+
def _dims_setter(self, value):
615+
self.elts = value
616+
617+
Tuple.dims = property(_dims_getter, _dims_setter)
611618

612619
class Suite(mod):
613620
"""Deprecated AST node class. Unused in Python 3."""

‎Lib/test/test_ast.py

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import ast
2+
import builtins
23
import dis
34
import os
45
import sys
6+
import types
57
import unittest
68
import warnings
79
import weakref
@@ -1945,6 +1947,88 @@ def visit_Ellipsis(self, node):
19451947
])
19461948

19471949

1950+
@support.cpython_only
1951+
class ModuleStateTests(unittest.TestCase):
1952+
# bpo-41194, bpo-41261, bpo-41631: The _ast module uses a global state.
1953+
1954+
def check_ast_module(self):
1955+
# Check that the _ast module still works as expected
1956+
code = 'x + 1'
1957+
filename = '<string>'
1958+
mode = 'eval'
1959+
1960+
# Create _ast.AST subclasses instances
1961+
ast_tree = compile(code, filename, mode, flags=ast.PyCF_ONLY_AST)
1962+
1963+
# Call PyAST_Check()
1964+
code = compile(ast_tree, filename, mode)
1965+
self.assertIsInstance(code, types.CodeType)
1966+
1967+
def test_reload_module(self):
1968+
# bpo-41194: Importing the _ast module twice must not crash.
1969+
with support.swap_item(sys.modules, '_ast', None):
1970+
del sys.modules['_ast']
1971+
import _ast as ast1
1972+
1973+
del sys.modules['_ast']
1974+
import _ast as ast2
1975+
1976+
self.check_ast_module()
1977+
1978+
# Unloading the two _ast module instances must not crash.
1979+
del ast1
1980+
del ast2
1981+
support.gc_collect()
1982+
1983+
self.check_ast_module()
1984+
1985+
def test_sys_modules(self):
1986+
# bpo-41631: Test reproducing a Mercurial crash when PyAST_Check()
1987+
# imported the _ast module internally.
1988+
lazy_mod = object()
1989+
1990+
def my_import(name, *args, **kw):
1991+
sys.modules[name] = lazy_mod
1992+
return lazy_mod
1993+
1994+
with support.swap_item(sys.modules, '_ast', None):
1995+
del sys.modules['_ast']
1996+
1997+
with support.swap_attr(builtins, '__import__', my_import):
1998+
# Test that compile() does not import the _ast module
1999+
self.check_ast_module()
2000+
self.assertNotIn('_ast', sys.modules)
2001+
2002+
# Sanity check of the test itself
2003+
import _ast
2004+
self.assertIs(_ast, lazy_mod)
2005+
2006+
def test_subinterpreter(self):
2007+
# bpo-41631: Importing and using the _ast module in a subinterpreter
2008+
# must not crash.
2009+
code = dedent('''
2010+
import _ast
2011+
import ast
2012+
import gc
2013+
import sys
2014+
import types
2015+
2016+
# Create _ast.AST subclasses instances and call PyAST_Check()
2017+
ast_tree = compile('x+1', '<string>', 'eval',
2018+
flags=ast.PyCF_ONLY_AST)
2019+
code = compile(ast_tree, 'string', 'eval')
2020+
if not isinstance(code, types.CodeType):
2021+
raise AssertionError
2022+
2023+
# Unloading the _ast module must not crash.
2024+
del ast, _ast
2025+
del sys.modules['ast'], sys.modules['_ast']
2026+
gc.collect()
2027+
''')
2028+
res = support.run_in_subinterp(code)
2029+
self.assertEqual(res, 0)
2030+
2031+
19482032
def main():
19492033
if __name__ != '__main__':
19502034
return

‎Misc/NEWS.d/next/Core and Builtins/2020-08-26-11-23-31.bpo-41631.3jZcd9.rst

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
The ``_ast`` module uses again a global state. Using a module state per module
2+
instance is causing subtle practical problems. For example, the Mercurial
3+
project replaces the ``__import__()`` function to implement lazy import,
4+
whereas Python expected that ``import _ast`` always return a fully initialized
5+
``_ast`` module.

‎Parser/asdl_c.py

Lines changed: 20 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1089,11 +1089,9 @@ def visitModule(self, mod):
10891089
static struct PyModuleDef _astmodule = {
10901090
PyModuleDef_HEAD_INIT,
10911091
.m_name = "_ast",
1092-
.m_size = sizeof(astmodulestate),
1092+
// The _ast module uses a global state (global_ast_state).
1093+
.m_size = 0,
10931094
.m_slots = astmodule_slots,
1094-
.m_traverse = astmodule_traverse,
1095-
.m_clear = astmodule_clear,
1096-
.m_free = astmodule_free,
10971095
};
10981096
10991097
PyMODINIT_FUNC
@@ -1374,59 +1372,40 @@ def generate_module_def(f, mod):
13741372
f.write(' PyObject *' + s + ';\n')
13751373
f.write('} astmodulestate;\n\n')
13761374
f.write("""
1377-
static astmodulestate*
1378-
get_ast_state(PyObject *module)
1379-
{
1380-
void *state = PyModule_GetState(module);
1381-
assert(state != NULL);
1382-
return (astmodulestate*)state;
1383-
}
1375+
// Forward declaration
1376+
static int init_types(astmodulestate *state);
1377+
1378+
// bpo-41194, bpo-41261, bpo-41631: The _ast module uses a global state.
1379+
static astmodulestate global_ast_state = {0};
13841380
13851381
static astmodulestate*
13861382
get_global_ast_state(void)
13871383
{
1388-
_Py_IDENTIFIER(_ast);
1389-
PyObject *name = _PyUnicode_FromId(&PyId__ast); // borrowed reference
1390-
if (name == NULL) {
1384+
astmodulestate* state = &global_ast_state;
1385+
if (!init_types(state)) {
13911386
return NULL;
13921387
}
1393-
PyObject *module = PyImport_GetModule(name);
1394-
if (module == NULL) {
1395-
if (PyErr_Occurred()) {
1396-
return NULL;
1397-
}
1398-
module = PyImport_Import(name);
1399-
if (module == NULL) {
1400-
return NULL;
1401-
}
1402-
}
1403-
astmodulestate *state = get_ast_state(module);
1404-
Py_DECREF(module);
14051388
return state;
14061389
}
14071390
1408-
static int astmodule_clear(PyObject *module)
1391+
static astmodulestate*
1392+
get_ast_state(PyObject* Py_UNUSED(module))
14091393
{
1410-
astmodulestate *state = get_ast_state(module);
1411-
""")
1412-
for s in module_state:
1413-
f.write(" Py_CLEAR(state->" + s + ');\n')
1414-
f.write("""
1415-
return 0;
1394+
astmodulestate* state = get_global_ast_state();
1395+
// get_ast_state() must only be called after _ast module is imported,
1396+
// and astmodule_exec() calls init_types()
1397+
assert(state != NULL);
1398+
return state;
14161399
}
14171400
1418-
static int astmodule_traverse(PyObject *module, visitproc visit, void* arg)
1401+
void _PyAST_Fini(PyThreadState *tstate)
14191402
{
1420-
astmodulestate *state = get_ast_state(module);
1403+
astmodulestate* state = &global_ast_state;
14211404
""")
14221405
for s in module_state:
1423-
f.write(" Py_VISIT(state->" + s + ');\n')
1406+
f.write(" Py_CLEAR(state->" + s + ');\n')
14241407
f.write("""
1425-
return 0;
1426-
}
1427-
1428-
static void astmodule_free(void* module) {
1429-
astmodule_clear((PyObject*)module);
1408+
state->initialized = 0;
14301409
}
14311410
14321411
""")

0 commit comments

Comments
 (0)
0