[mypyc] Fix segfault when top level raises exception #10586
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Change the test added in c39bad8 to verify that importing native a second time should fail with an AssertionError due to the `assert False` in native.
If an error occurs while running the initialization code, set the CPyModule_<name>_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.
msullivan
approved these changes
Jun 8, 2021
There was a problem hiding this comment.
OK, this looks great, and is much better behavior than segfaulting.
Thinking about it a bit more, the fully right thing to do would be to also free the module and any types/statics that were created before the error occured.
I'm going to merge this now, but could you file a follow-up issue about doing that? (Actually doing it is not super high priority)
msullivan
pushed a commit
that referenced
this pull request
Jun 13, 2021
This frees everything created in the generated initialization function so that we don't leak memory in the case of an error during initialization, as mentioned in #10586 (review).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes mypyc/mypyc#813
This prevents the compiled code from segfaulting when the top level code raises an exception. In that case, previously, the compiled code would store an internal pointer to the module object and would just return that pointer if the same module was initialized more than once. However, if an exception was raised while running top level code, the internal pointer would still point to the module object even though not everything was completely initialized, leading future attempts to import that module to use functions and classes that were never completely initialized.
This fixes that bug by resetting the internal module pointer to NULL when an error occurs so that the top level code is run again on future attempts to import that same function.
Test Plan
I added a run test to run-misc.test that reproduces this segfault and made sure it passed after my changes.
I wasn't really sure where I should put that test so I ended up putting it in run-misc.test, but let me know if you want me to move it somewhere else.