Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add_shell: get __builtins__ directly instead of from __main__ #261

Closed
wants to merge 1 commit into from

Conversation

quarl
Copy link

@quarl quarl commented Mar 21, 2019

In some situations, main.builtins doesn't exist at the time
add_shell() is called. The old code assumed that it does, and would segfault.

Before this commit:

$ python -c 'import IPython; \
             app = IPython.terminal.ipapp.TerminalIPythonApp.instance(); \
             app.user_ns = {}; app.initialize(); app.start()'
In [1]: import apsw
<segmentation fault>

This was first discovered when using pyflyby:

$ py
In [1]: import apsw
<segmentation fault>

To avoid that problem, this commit changes the code to import builtin as a
module directly.

In some situations, __main__.__builtins__ doesn't exist at the time
add_shell() is called.  The old code assumed that it does, and would segfault.

Before this commit:
$ python -c 'import IPython; \
             app = IPython.terminal.ipapp.TerminalIPythonApp.instance(); \
             app.user_ns = {}; app.initialize(); app.start()'
In [1]: import apsw
<segmentation fault>

This was first discovered when using pyflyby:
$ py
In [1]: import apsw
<segmentation fault>

To avoid that problem, this commit changes the code to import __builtin__ as a
module directly.
@rogerbinns
Copy link
Owner

This patch exposes a different problem. In some situations, the builtins module doesn't have import yet. You can see if you run tools/megatest.py which compiles fresh python versions and runs the test suite. The symptom is this when running the tests:

Traceback (most recent call last):
  File "<string>", line 2, in <module>
ImportError: __import__ not found

Thank you for finding this issue and I would like to fix it, but it is going to take more work from me first.

@rogerbinns
Copy link
Owner

The good news is that the shell is now broken out from the extension (#356) so this is no longer an issue. apsw will import just fine,

@rogerbinns rogerbinns closed this Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants