From c39bad8a0105b62fa2dde22686a92d207b26c8b9 Mon Sep 17 00:00:00 2001 From: Pranav Rajpal <78008260+pranavrajpal@users.noreply.github.com> Date: Thu, 3 Jun 2021 20:29:57 -0700 Subject: [PATCH 1/3] Add test to reproduce segfault Adds a test that reproduces the segfault that occurs on CI runs due to a exception being raised while trying to import a module that later gets used. --- mypyc/test-data/run-misc.test | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/mypyc/test-data/run-misc.test b/mypyc/test-data/run-misc.test index 1df800ee6560..d4b37f0cf974 100644 --- a/mypyc/test-data/run-misc.test +++ b/mypyc/test-data/run-misc.test @@ -1126,3 +1126,23 @@ C = sys.platform == 'x' and (lambda x: y + x) assert not A assert not B assert not C + +[case testDoesntSegfaultWhenTopLevelFails] +# make the initial import fail +assert False + +class C: + def __init__(self): + self.x = 1 + self.y = 2 +def test() -> None: + a = C() +[file driver.py] +# load native, cause PyInit to be run, create the module but don't finish initializing the globals +try: + import native +except: + pass +# try accessing those globals that were never properly initialized +import native +native.test() From c6b6d86cae95bb0188769715069a0293c67f5474 Mon Sep 17 00:00:00 2001 From: Pranav Rajpal <78008260+pranavrajpal@users.noreply.github.com> Date: Thu, 3 Jun 2021 21:11:35 -0700 Subject: [PATCH 2/3] Change test to correctly verify expected outcome Change the test added in c39bad8a0105b62fa2dde22686a92d207b26c8b9 to verify that importing native a second time should fail with an AssertionError due to the `assert False` in native. --- mypyc/test-data/run-misc.test | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/mypyc/test-data/run-misc.test b/mypyc/test-data/run-misc.test index d4b37f0cf974..dd69e42e9373 100644 --- a/mypyc/test-data/run-misc.test +++ b/mypyc/test-data/run-misc.test @@ -1143,6 +1143,10 @@ try: import native except: pass -# try accessing those globals that were never properly initialized -import native -native.test() +try: + # try accessing those globals that were never properly initialized + import native + native.test() +# should fail with AssertionError due to `assert False` in other function +except AssertionError: + pass From b2a05b53d0653a1fd74f78797b747887a2bdabc1 Mon Sep 17 00:00:00 2001 From: Pranav Rajpal <78008260+pranavrajpal@users.noreply.github.com> Date: Mon, 7 Jun 2021 00:15:22 -0700 Subject: [PATCH 3/3] Reset internal module pointer to NULL on error If an error occurs while running the initialization code, set the CPyModule__internal module pointer to NULL so future attempts to import that same module don't mistakenly think that the module is already initialized due to the fact that that module pointer is not NULL. Clearing that module pointer on error allows us to keep initializing it at the beginning of the function before running any top level code, which is necessary to prevent RecursionErrors when dealing with circular imports. --- mypyc/codegen/emitmodule.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/mypyc/codegen/emitmodule.py b/mypyc/codegen/emitmodule.py index 476c435712bd..5a495ec0b61c 100644 --- a/mypyc/codegen/emitmodule.py +++ b/mypyc/codegen/emitmodule.py @@ -888,7 +888,7 @@ def generate_module_def(self, emitter: Emitter, module_name: str, module: Module emitter.emit_lines('{} = PyModule_Create(&{}module);'.format(module_static, module_prefix), 'if (unlikely({} == NULL))'.format(module_static), - ' return NULL;') + ' goto fail;') emitter.emit_line( 'PyObject *modname = PyObject_GetAttrString((PyObject *){}, "__name__");'.format( module_static)) @@ -896,7 +896,7 @@ def generate_module_def(self, emitter: Emitter, module_name: str, module: Module module_globals = emitter.static_name('globals', module_name) emitter.emit_lines('{} = PyModule_GetDict({});'.format(module_globals, module_static), 'if (unlikely({} == NULL))'.format(module_globals), - ' return NULL;') + ' goto fail;') # HACK: Manually instantiate generated classes here for cl in module.classes: @@ -907,16 +907,19 @@ def generate_module_def(self, emitter: Emitter, module_name: str, module: Module '(PyObject *){t}_template, NULL, modname);' .format(t=type_struct)) emitter.emit_lines('if (unlikely(!{}))'.format(type_struct), - ' return NULL;') + ' goto fail;') emitter.emit_lines('if (CPyGlobalsInit() < 0)', - ' return NULL;') + ' goto fail;') self.generate_top_level_call(module, emitter) emitter.emit_lines('Py_DECREF(modname);') emitter.emit_line('return {};'.format(module_static)) + emitter.emit_lines('fail:', + '{} = NULL;'.format(module_static), + 'return NULL;') emitter.emit_line('}') def generate_top_level_call(self, module: ModuleIR, emitter: Emitter) -> None: @@ -927,7 +930,7 @@ def generate_top_level_call(self, module: ModuleIR, emitter: Emitter) -> None: emitter.emit_lines( 'char result = {}();'.format(emitter.native_function_name(fn.decl)), 'if (result == 2)', - ' return NULL;', + ' goto fail;', ) break