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

gh-112292 : Catch import error conditions with readline hooks #112313

Merged
merged 19 commits into from Nov 28, 2023

Conversation

tonybaloney
Copy link
Contributor

@tonybaloney tonybaloney commented Nov 22, 2023

Prevents a segmentation fault when modules import (or configure) readline from inside a sub interpreter.

The readlinestate_global macro was error-prone to PyImport_FindModule returning NULL and crashing in about 18 places. I could reproduce 1 easily, but this PR replaces the macro with a function and adds error conditions to the other functions.

Modules/readline.c Outdated Show resolved Hide resolved
@ericsnowcurrently
Copy link
Member

+1 to fixing the other hooks, whether in this PR or another (though I'd think this PR would be the better place).

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM. I've noted one thing to fix and a couple others to consider.

Modules/readline.c Outdated Show resolved Hide resolved
Modules/readline.c Show resolved Hide resolved
Modules/readline.c Outdated Show resolved Hide resolved
Modules/readline.c Outdated Show resolved Hide resolved
@bedevere-app
Copy link

bedevere-app bot commented Nov 22, 2023

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@tonybaloney tonybaloney changed the title gh-112292 : Catch import error conditions with readline on_startup_hook gh-112292 : Catch import error conditions with readline hooks Nov 23, 2023
@tonybaloney
Copy link
Contributor Author

I have made the requested changes; please review again

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the extra effort here. There are a few things we may want to do differently here, for which I've left notes.

Modules/readline.c Outdated Show resolved Hide resolved
Modules/readline.c Outdated Show resolved Hide resolved
Modules/readline.c Outdated Show resolved Hide resolved
Modules/readline.c Outdated Show resolved Hide resolved
Modules/readline.c Outdated Show resolved Hide resolved
Modules/readline.c Outdated Show resolved Hide resolved
Modules/readline.c Show resolved Hide resolved
Modules/readline.c Outdated Show resolved Hide resolved
Modules/readline.c Outdated Show resolved Hide resolved
Modules/readline.c Outdated Show resolved Hide resolved
tonybaloney and others added 7 commits November 28, 2023 10:04
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
@tonybaloney
Copy link
Contributor Author

@ericsnowcurrently it wasn't possible to check module state inside setup_readline but this code is more robust now so it no longer crashes when imported twice in a sub interpreter. test_builtin and test_readline now correctly fail instead of segfaulting.

$ ./python.exe test_in_interp.py test_builtin
Running test test_builtin
test_abs (test.test_builtin.BuiltinTest.test_abs) ... ok
test_all (test.test_builtin.BuiltinTest.test_all) ... ok
test_any (test.test_builtin.BuiltinTest.test_any) ... ok
test_ascii (test.test_builtin.BuiltinTest.test_ascii) ... ok
test_bin (test.test_builtin.BuiltinTest.test_bin) ... ok
test_bug_27936 (test.test_builtin.BuiltinTest.test_bug_27936) ... ok
test_bytearray_extend_error (test.test_builtin.BuiltinTest.test_bytearray_extend_error) ... ok
test_bytearray_translate (test.test_builtin.BuiltinTest.test_bytearray_translate) ... ok
test_callable (test.test_builtin.BuiltinTest.test_callable) ... ok
test_chr (test.test_builtin.BuiltinTest.test_chr) ... ok
test_cmp (test.test_builtin.BuiltinTest.test_cmp) ... ok
test_compile (test.test_builtin.BuiltinTest.test_compile) ... ok
test_compile_ast (test.test_builtin.BuiltinTest.test_compile_ast) ... ok
test_compile_async_generator (test.test_builtin.BuiltinTest.test_compile_async_generator)
With the PyCF_ALLOW_TOP_LEVEL_AWAIT flag added in 3.8, we want to ... ok
test_compile_top_level_await (test.test_builtin.BuiltinTest.test_compile_top_level_await)
Test whether code some top level await can be compiled. ... ok
test_compile_top_level_await_invalid_cases (test.test_builtin.BuiltinTest.test_compile_top_level_await_invalid_cases) ... ok
test_compile_top_level_await_no_coro (test.test_builtin.BuiltinTest.test_compile_top_level_await_no_coro)
Make sure top level non-await codes get the correct coroutine flags ... ok
test_construct_singletons (test.test_builtin.BuiltinTest.test_construct_singletons) ... ok
test_delattr (test.test_builtin.BuiltinTest.test_delattr) ... ok
test_dir (test.test_builtin.BuiltinTest.test_dir) ... ok
test_divmod (test.test_builtin.BuiltinTest.test_divmod) ... ok
test_eval (test.test_builtin.BuiltinTest.test_eval) ... ok
test_exec (test.test_builtin.BuiltinTest.test_exec) ... ok
test_exec_closure (test.test_builtin.BuiltinTest.test_exec_closure) ... ok
test_exec_globals (test.test_builtin.BuiltinTest.test_exec_globals) ... ok
test_exec_globals_dict_subclass (test.test_builtin.BuiltinTest.test_exec_globals_dict_subclass) ... ok
test_exec_globals_error_on_get (test.test_builtin.BuiltinTest.test_exec_globals_error_on_get) ... ok
test_exec_globals_frozen (test.test_builtin.BuiltinTest.test_exec_globals_frozen) ... ok
test_exec_redirected (test.test_builtin.BuiltinTest.test_exec_redirected) ... ok
test_filter (test.test_builtin.BuiltinTest.test_filter) ... ok
test_filter_dealloc (test.test_builtin.BuiltinTest.test_filter_dealloc) ... skipped "resource 'cpu' is not enabled"
test_filter_pickle (test.test_builtin.BuiltinTest.test_filter_pickle) ... ok
test_format (test.test_builtin.BuiltinTest.test_format) ... ok
test_general_eval (test.test_builtin.BuiltinTest.test_general_eval) ... ok
test_getattr (test.test_builtin.BuiltinTest.test_getattr) ... ok
test_hasattr (test.test_builtin.BuiltinTest.test_hasattr) ... ok
test_hash (test.test_builtin.BuiltinTest.test_hash) ... ok
test_hex (test.test_builtin.BuiltinTest.test_hex) ... ok
test_id (test.test_builtin.BuiltinTest.test_id) ... ok
test_import (test.test_builtin.BuiltinTest.test_import) ... ok
test_input (test.test_builtin.BuiltinTest.test_input) ... ok
test_isinstance (test.test_builtin.BuiltinTest.test_isinstance) ... ok
test_issubclass (test.test_builtin.BuiltinTest.test_issubclass) ... ok
test_iter (test.test_builtin.BuiltinTest.test_iter) ... ok
test_len (test.test_builtin.BuiltinTest.test_len) ... ok
test_map (test.test_builtin.BuiltinTest.test_map) ... ok
test_map_pickle (test.test_builtin.BuiltinTest.test_map_pickle) ... ok
test_max (test.test_builtin.BuiltinTest.test_max) ... ok
test_min (test.test_builtin.BuiltinTest.test_min) ... ok
test_neg (test.test_builtin.BuiltinTest.test_neg) ... ok
test_next (test.test_builtin.BuiltinTest.test_next) ... ok
test_oct (test.test_builtin.BuiltinTest.test_oct) ... ok
test_open (test.test_builtin.BuiltinTest.test_open) ... ok
test_open_default_encoding (test.test_builtin.BuiltinTest.test_open_default_encoding) ... ok
test_open_non_inheritable (test.test_builtin.BuiltinTest.test_open_non_inheritable) ... ok
test_ord (test.test_builtin.BuiltinTest.test_ord) ... ok
test_pow (test.test_builtin.BuiltinTest.test_pow) ... ok
test_repr (test.test_builtin.BuiltinTest.test_repr) ... ok
test_round (test.test_builtin.BuiltinTest.test_round) ... ok
test_round_large (test.test_builtin.BuiltinTest.test_round_large) ... ok
test_setattr (test.test_builtin.BuiltinTest.test_setattr) ... ok
test_sum (test.test_builtin.BuiltinTest.test_sum) ... ok
test_sum_accuracy (test.test_builtin.BuiltinTest.test_sum_accuracy) ... ok
test_type (test.test_builtin.BuiltinTest.test_type) ... ok
test_vars (test.test_builtin.BuiltinTest.test_vars) ... ok
test_warning_notimplemented (test.test_builtin.BuiltinTest.test_warning_notimplemented) ... ok
test_zip (test.test_builtin.BuiltinTest.test_zip) ... ok
test_zip_bad_iterable (test.test_builtin.BuiltinTest.test_zip_bad_iterable) ... ok
test_zip_pickle (test.test_builtin.BuiltinTest.test_zip_pickle) ... ok
test_zip_pickle_strict (test.test_builtin.BuiltinTest.test_zip_pickle_strict) ... ok
test_zip_pickle_strict_fail (test.test_builtin.BuiltinTest.test_zip_pickle_strict_fail) ... ok
test_zip_result_gc (test.test_builtin.BuiltinTest.test_zip_result_gc) ... ok
test_zip_strict (test.test_builtin.BuiltinTest.test_zip_strict) ... ok
test_zip_strict_error_handling (test.test_builtin.BuiltinTest.test_zip_strict_error_handling) ... ok
test_zip_strict_error_handling_stopiteration (test.test_builtin.BuiltinTest.test_zip_strict_error_handling_stopiteration) ... ok
test_zip_strict_iterators (test.test_builtin.BuiltinTest.test_zip_strict_iterators) ... ok
test_immortals (test.test_builtin.ImmortalTests.test_immortals) ... ok
test_list_repeat_respect_immortality (test.test_builtin.ImmortalTests.test_list_repeat_respect_immortality) ... ok
test_tuple_repeat_respect_immortality (test.test_builtin.ImmortalTests.test_tuple_repeat_respect_immortality) ... ok
test_input_no_stdout_fileno (test.test_builtin.PtyTests.test_input_no_stdout_fileno) ... ERROR
test_input_tty (test.test_builtin.PtyTests.test_input_tty) ... ERROR
test_input_tty_non_ascii (test.test_builtin.PtyTests.test_input_tty_non_ascii) ... ERROR
test_input_tty_non_ascii_unicode_errors (test.test_builtin.PtyTests.test_input_tty_non_ascii_unicode_errors) ... ERROR
test_cleanup (test.test_builtin.ShutdownTest.test_cleanup) ... ok
test_breakpoint (test.test_builtin.TestBreakpoint.test_breakpoint) ... ok
test_breakpoint_with_args_and_keywords (test.test_builtin.TestBreakpoint.test_breakpoint_with_args_and_keywords) ... ok
test_breakpoint_with_breakpointhook_reset (test.test_builtin.TestBreakpoint.test_breakpoint_with_breakpointhook_reset) ... ok
test_breakpoint_with_breakpointhook_set (test.test_builtin.TestBreakpoint.test_breakpoint_with_breakpointhook_set) ... ok
test_breakpoint_with_passthru_error (test.test_builtin.TestBreakpoint.test_breakpoint_with_passthru_error) ... ok
test_envar_good_path_builtin (test.test_builtin.TestBreakpoint.test_envar_good_path_builtin) ... ok
test_envar_good_path_empty_string (test.test_builtin.TestBreakpoint.test_envar_good_path_empty_string) ... ok
test_envar_good_path_noop_0 (test.test_builtin.TestBreakpoint.test_envar_good_path_noop_0) ... ok
test_envar_good_path_other (test.test_builtin.TestBreakpoint.test_envar_good_path_other) ... ok
test_envar_ignored_when_hook_is_set (test.test_builtin.TestBreakpoint.test_envar_ignored_when_hook_is_set) ... ok
test_envar_unimportable (test.test_builtin.TestBreakpoint.test_envar_unimportable) ... ok
test_runtime_error_when_hook_is_lost (test.test_builtin.TestBreakpoint.test_runtime_error_when_hook_is_lost) ... ok
test_bad_arguments (test.test_builtin.TestSorted.test_bad_arguments) ... ok
test_baddecorator (test.test_builtin.TestSorted.test_baddecorator) ... ok
test_basic (test.test_builtin.TestSorted.test_basic) ... ok
test_inputtypes (test.test_builtin.TestSorted.test_inputtypes) ... ok
test_bad_args (test.test_builtin.TestType.test_bad_args) ... ok
test_bad_slots (test.test_builtin.TestType.test_bad_slots) ... ok
test_namespace_order (test.test_builtin.TestType.test_namespace_order) ... ok
test_new_type (test.test_builtin.TestType.test_new_type) ... ok
test_type_doc (test.test_builtin.TestType.test_type_doc) ... ok
test_type_name (test.test_builtin.TestType.test_type_name) ... ok
test_type_nokwargs (test.test_builtin.TestType.test_type_nokwargs) ... ok
test_type_qualname (test.test_builtin.TestType.test_type_qualname) ... ok
test_type_typeparams (test.test_builtin.TestType.test_type_typeparams) ... ok
bin (builtins)
Doctest: builtins.bin ... ok
hex (builtins.bytearray)
Doctest: builtins.bytearray.hex ... ok
hex (builtins.bytes)
Doctest: builtins.bytes.hex ... ok
as_integer_ratio (builtins.float)
Doctest: builtins.float.as_integer_ratio ... ok
fromhex (builtins.float)
Doctest: builtins.float.fromhex ... ok
hex (builtins.float)
Doctest: builtins.float.hex ... ok
hex (builtins)
Doctest: builtins.hex ... ok
int (builtins)
Doctest: builtins.int ... ok
as_integer_ratio (builtins.int)
Doctest: builtins.int.as_integer_ratio ... ok
bit_count (builtins.int)
Doctest: builtins.int.bit_count ... ok
bit_length (builtins.int)
Doctest: builtins.int.bit_length ... ok
hex (builtins.memoryview)
Doctest: builtins.memoryview.hex ... ok
oct (builtins)
Doctest: builtins.oct ... ok
zip (builtins)
Doctest: builtins.zip ... ok

======================================================================
ERROR: test_input_no_stdout_fileno (test.test_builtin.PtyTests.test_input_no_stdout_fileno)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2314, in test_input_no_stdout_fileno
    lines = self.run_child(child, b"quux\r")
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2187, in run_child
    old_sighup = signal.signal(signal.SIGHUP, self.handle_sighup)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/anthonyshaw/projects/cpython/Lib/signal.py", line 56, in signal
    handler = _signal.signal(_enum_to_int(signalnum), _enum_to_int(handler))
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: signal only works in main thread of the main interpreter

======================================================================
ERROR: test_input_tty (test.test_builtin.PtyTests.test_input_tty)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2284, in test_input_tty
    self.check_input_tty("prompt", b"quux")
  File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2269, in check_input_tty
    lines = self.run_child(child, terminal_input + b"\r\n")
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2187, in run_child
    old_sighup = signal.signal(signal.SIGHUP, self.handle_sighup)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/anthonyshaw/projects/cpython/Lib/signal.py", line 56, in signal
    handler = _signal.signal(_enum_to_int(signalnum), _enum_to_int(handler))
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: signal only works in main thread of the main interpreter

======================================================================
ERROR: test_input_tty_non_ascii (test.test_builtin.PtyTests.test_input_tty_non_ascii)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2299, in test_input_tty_non_ascii
    self.check_input_tty("prompté", b"quux\xe9", "utf-8")
  File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2269, in check_input_tty
    lines = self.run_child(child, terminal_input + b"\r\n")
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2187, in run_child
    old_sighup = signal.signal(signal.SIGHUP, self.handle_sighup)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/anthonyshaw/projects/cpython/Lib/signal.py", line 56, in signal
    handler = _signal.signal(_enum_to_int(signalnum), _enum_to_int(handler))
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: signal only works in main thread of the main interpreter

======================================================================
ERROR: test_input_tty_non_ascii_unicode_errors (test.test_builtin.PtyTests.test_input_tty_non_ascii_unicode_errors)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2304, in test_input_tty_non_ascii_unicode_errors
    self.check_input_tty("prompté", b"quux\xe9", "ascii")
  File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2269, in check_input_tty
    lines = self.run_child(child, terminal_input + b"\r\n")
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2187, in run_child
    old_sighup = signal.signal(signal.SIGHUP, self.handle_sighup)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/anthonyshaw/projects/cpython/Lib/signal.py", line 56, in signal
    handler = _signal.signal(_enum_to_int(signalnum), _enum_to_int(handler))
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: signal only works in main thread of the main interpreter

----------------------------------------------------------------------
Ran 122 tests in 0.298s

FAILED (errors=4, skipped=1)
test test_builtin failed
$ ./python.exe test_in_interp.py test_readline
Running test test_readline
test_readline skipped -- module readline does not support loading in subinterpreters

@ericsnowcurrently
Copy link
Member

it wasn't possible to check module state inside setup_readline

I'm guessing it's because PyState_FindModule() will always return NULL until after the module init func has already finished. That's fine.

but this code is more robust now so it no longer crashes when imported twice in a sub interpreter.

Yeah, that's the higher priority thing. The module is definitely in a better place now.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ericsnowcurrently ericsnowcurrently merged commit 154f099 into python:main Nov 28, 2023
31 of 32 checks passed
@ericsnowcurrently
Copy link
Member

Thanks for taking care of this, @tonybaloney!

@ericsnowcurrently
Copy link
Member

How much of this should be backported to 3.12?

@tonybaloney
Copy link
Contributor Author

How much of this should be backported to 3.12?

All of it could be

@tonybaloney tonybaloney deleted the robust_readline_hook branch November 28, 2023 02:01
@ericsnowcurrently
Copy link
Member

@Yhg1s, what do you think about backporting this as-is?

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…ythongh-112313)

Prevents a segmentation fault in registered hooks for the readline library, but only when the readline module is loaded inside an isolated sub interpreter.  The module is single-phase init so loading it fails, but not until the module init function has already run, where the readline hooks get registered.

The readlinestate_global macro was error-prone to PyImport_FindModule returning NULL and crashing in about 18 places.  I could reproduce 1 easily, but this PR replaces the macro with a function and adds error conditions to the other functions.
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

3 participants