diff --git a/plugins/python/pyhelpers.c b/plugins/python/pyhelpers.c index aae1eb071..d7b48f848 100644 --- a/plugins/python/pyhelpers.c +++ b/plugins/python/pyhelpers.c @@ -63,10 +63,7 @@ _sudo_printf_default(int msg_type, const char *fmt, ...) struct PythonContext py_ctx = { - &_sudo_printf_default, - NULL, - 0, - NULL + .sudo_log = &_sudo_printf_default, }; diff --git a/plugins/python/pyhelpers.h b/plugins/python/pyhelpers.h index b6d25a44a..fe4810bde 100644 --- a/plugins/python/pyhelpers.h +++ b/plugins/python/pyhelpers.h @@ -38,12 +38,15 @@ enum SudoPluginFunctionReturnCode { SUDO_RC_USAGE_ERROR = -2, }; +#define INTERPRETER_MAX 32 + struct PythonContext { sudo_printf_t sudo_log; sudo_conv_t sudo_conv; - int open_plugin_count; PyThreadState *py_main_interpreter; + size_t interpreter_count; + PyThreadState *py_subinterpreters[INTERPRETER_MAX]; }; extern struct PythonContext py_ctx; diff --git a/plugins/python/python_plugin_common.c b/plugins/python/python_plugin_common.c index fa7c4db7b..37494cebc 100644 --- a/plugins/python/python_plugin_common.c +++ b/plugins/python/python_plugin_common.c @@ -110,6 +110,24 @@ _import_module(const char *path) debug_return_ptr(PyImport_ImportModule(module_name)); } +static PyThreadState * +_python_plugin_new_interpreter(void) +{ + debug_decl(_python_plugin_new_interpreter, PYTHON_DEBUG_INTERNAL); + if (py_ctx.interpreter_count >= INTERPRETER_MAX) { + PyErr_Format(PyExc_Exception, "Too many interpreters"); + debug_return_ptr(NULL); + } + + PyThreadState *py_interpreter = Py_NewInterpreter(); + if (py_interpreter != NULL) { + py_ctx.py_subinterpreters[py_ctx.interpreter_count] = py_interpreter; + ++py_ctx.interpreter_count; + } + + debug_return_ptr(py_interpreter); +} + void python_plugin_handle_plugin_error_exception(PyObject **py_result, struct PluginContext *plugin_ctx) { @@ -247,7 +265,7 @@ python_plugin_register_logging(sudo_conv_t conversation, debug_decl(python_plugin_register_logging, PYTHON_DEBUG_INTERNAL); int rc = SUDO_RC_ERROR; - if (py_ctx.sudo_conv == NULL) + if (conversation != NULL) py_ctx.sudo_conv = conversation; if (sudo_printf) @@ -287,8 +305,6 @@ _python_plugin_register_plugin_in_py_ctx(void) debug_decl(_python_plugin_register_plugin_in_py_ctx, PYTHON_DEBUG_PLUGIN_LOAD); if (!Py_IsInitialized()) { - py_ctx.open_plugin_count = 0; - // Disable environment variables effecting the python interpreter // This is important since we are running code here as root, the // user should not be able to alter what is running any how. @@ -312,7 +328,6 @@ _python_plugin_register_plugin_in_py_ctx(void) PyThreadState_Swap(py_ctx.py_main_interpreter); } - ++py_ctx.open_plugin_count; debug_return_int(SUDO_RC_OK); } @@ -329,7 +344,7 @@ python_plugin_init(struct PluginContext *plugin_ctx, char * const plugin_options plugin_ctx->sudo_api_version = version; - plugin_ctx->py_interpreter = Py_NewInterpreter(); + plugin_ctx->py_interpreter = _python_plugin_new_interpreter(); if (plugin_ctx->py_interpreter == NULL) { goto cleanup; } @@ -388,36 +403,22 @@ void python_plugin_deinit(struct PluginContext *plugin_ctx) { debug_decl(python_plugin_deinit, PYTHON_DEBUG_PLUGIN_LOAD); - --py_ctx.open_plugin_count; - sudo_debug_printf(SUDO_DEBUG_DIAG, "Closing: %d python plugins left open\n", py_ctx.open_plugin_count); + sudo_debug_printf(SUDO_DEBUG_DIAG, "Deinit was called for a python plugin\n"); Py_CLEAR(plugin_ctx->py_instance); Py_CLEAR(plugin_ctx->py_class); Py_CLEAR(plugin_ctx->py_module); - if (plugin_ctx->py_interpreter != NULL) { - sudo_debug_printf(SUDO_DEBUG_TRACE, "deinit python interpreter for plugin\n"); - Py_EndInterpreter(plugin_ctx->py_interpreter); - } + // Note: we are preserving the interpreters here until the unlink because + // of bugs like (strptime does not work after python interpreter reinit): + // https://bugs.python.org/issue27400 + // These potentially effect a lot more python functions, simply because + // it is a rare tested scenario. free(plugin_ctx->callback_error); memset(plugin_ctx, 0, sizeof(*plugin_ctx)); - if (py_ctx.open_plugin_count <= 0) { - if (Py_IsInitialized()) { - sudo_debug_printf(SUDO_DEBUG_NOTICE, "Closing: deinit python interpreter\n"); - - // we need to call finalize from the main interpreter - PyThreadState_Swap(py_ctx.py_main_interpreter); - - Py_Finalize(); - } - - py_ctx_reset(); - } - python_debug_deregister(); - debug_return; } @@ -551,3 +552,35 @@ python_plugin_name(struct PluginContext *plugin_ctx) debug_return_const_str(((PyTypeObject *)(plugin_ctx->py_class))->tp_name); } + +void python_plugin_unlink(void) __attribute__((destructor)); + +// this gets run only when sudo unlinks the python_plugin.so +void +python_plugin_unlink(void) +{ + debug_decl(python_plugin_unlink, PYTHON_DEBUG_INTERNAL); + if (py_ctx.py_main_interpreter == NULL) + return; + + if (Py_IsInitialized()) { + sudo_debug_printf(SUDO_DEBUG_NOTICE, "Closing: deinit python %lu subinterpreters\n", + py_ctx.interpreter_count); + for (size_t i = 0; i < py_ctx.interpreter_count; ++i) { + PyThreadState *py_interpreter = py_ctx.py_subinterpreters[i]; + PyThreadState_Swap(py_interpreter); + Py_EndInterpreter(py_interpreter); + } + + sudo_debug_printf(SUDO_DEBUG_NOTICE, "Closing: deinit main interpreter\n"); + + // we need to call finalize from the main interpreter + PyThreadState_Swap(py_ctx.py_main_interpreter); + + if (Py_FinalizeEx() != 0) { + sudo_debug_printf(SUDO_DEBUG_WARN, "Closing: failed to deinit python interpreter\n"); + } + } + py_ctx_reset(); + debug_return; +} diff --git a/plugins/python/regress/check_python_examples.c b/plugins/python/regress/check_python_examples.c index ba38ce297..484cfcb4e 100644 --- a/plugins/python/regress/check_python_examples.c +++ b/plugins/python/regress/check_python_examples.c @@ -25,18 +25,28 @@ #include "sudo_dso.h" +#define DECL_PLUGIN(type, variable_name) \ + static struct type *variable_name = NULL; \ + static struct type variable_name ## _original + +#define RESTORE_PYTHON_PLUGIN(variable_name) \ + memcpy(variable_name, &(variable_name ## _original), sizeof(variable_name ## _original)) + +#define SAVE_PYTHON_PLUGIN(variable_name) \ + memcpy(&(variable_name ## _original), variable_name, sizeof(variable_name ## _original)) + static const char *python_plugin_so_path = NULL; static void *python_plugin_handle = NULL; -static struct io_plugin *python_io = NULL; -static struct policy_plugin *python_policy = NULL; -static struct approval_plugin *python_approval = NULL; -static struct sudoers_group_plugin *group_plugin = NULL; -static struct audit_plugin *python_audit = NULL; +DECL_PLUGIN(io_plugin, python_io); +DECL_PLUGIN(policy_plugin, python_policy); +DECL_PLUGIN(approval_plugin, python_approval); +DECL_PLUGIN(audit_plugin, python_audit); +DECL_PLUGIN(sudoers_group_plugin, group_plugin); static struct passwd example_pwd; -static int _load_symbols(void); -static int _unload_symbols(void); +static int _init_symbols(void); +static int _unlink_symbols(void); void create_plugin_options(const char *module_name, const char *class_name, const char *extra_option) @@ -119,7 +129,7 @@ init(void) data.plugin_argv = create_str_array(1, NULL); data.user_env = create_str_array(1, NULL); - VERIFY_TRUE(_load_symbols()); + VERIFY_TRUE(_init_symbols()); return true; } @@ -146,9 +156,6 @@ cleanup(int success) str_array_free(&data.user_env); str_array_free(&data.plugin_options); - VERIFY_FALSE(Py_IsInitialized()); - - VERIFY_TRUE(_unload_symbols()); return true; } @@ -507,6 +514,18 @@ check_example_group_plugin_is_able_to_debug(void) return true; } +int +check_plugin_unload(void) +{ + // You can call this test to avoid having a lot of subinterpreters + // (each plugin->open starts one, and only plugin unlink closes) + // It only verifies that python was shut down correctly. + VERIFY_TRUE(Py_IsInitialized()); + VERIFY_TRUE(_unlink_symbols()); + VERIFY_FALSE(Py_IsInitialized()); // python interpreter could be stopped + return true; +} + int check_example_debugging(const char *debug_spec) { @@ -1403,8 +1422,19 @@ check_multiple_approval_plugin_and_arguments(void) static int -_load_symbols(void) +_init_symbols(void) { + if (python_plugin_handle != NULL) { + // symbols are already loaded, we just restore + RESTORE_PYTHON_PLUGIN(python_io); + RESTORE_PYTHON_PLUGIN(python_policy); + RESTORE_PYTHON_PLUGIN(python_approval); + RESTORE_PYTHON_PLUGIN(python_audit); + RESTORE_PYTHON_PLUGIN(group_plugin); + return true; + } + + // we load the symbols python_plugin_handle = sudo_dso_load(python_plugin_so_path, SUDO_DSO_LAZY|SUDO_DSO_GLOBAL); VERIFY_PTR_NE(python_plugin_handle, NULL); @@ -1423,11 +1453,17 @@ _load_symbols(void) python_approval = sudo_dso_findsym(python_plugin_handle, "python_approval"); VERIFY_PTR_NE(python_approval, NULL); + SAVE_PYTHON_PLUGIN(python_io); + SAVE_PYTHON_PLUGIN(python_policy); + SAVE_PYTHON_PLUGIN(python_approval); + SAVE_PYTHON_PLUGIN(python_audit); + SAVE_PYTHON_PLUGIN(group_plugin); + return true; } static int -_unload_symbols(void) +_unlink_symbols(void) { python_io = NULL; group_plugin = NULL; @@ -1435,10 +1471,11 @@ _unload_symbols(void) python_approval = NULL; python_audit = NULL; VERIFY_INT(sudo_dso_unload(python_plugin_handle), 0); + python_plugin_handle = NULL; + VERIFY_FALSE(Py_IsInitialized()); return true; } - int main(int argc, char *argv[]) { @@ -1456,19 +1493,23 @@ main(int argc, char *argv[]) RUN_TEST(check_example_io_plugin_fails_with_python_backtrace()); RUN_TEST(check_io_plugin_callbacks_are_optional()); RUN_TEST(check_io_plugin_reports_error()); + RUN_TEST(check_plugin_unload()); RUN_TEST(check_example_group_plugin()); RUN_TEST(check_example_group_plugin_is_able_to_debug()); + RUN_TEST(check_plugin_unload()); RUN_TEST(check_loading_fails_with_missing_path()); RUN_TEST(check_loading_fails_with_missing_classname()); RUN_TEST(check_loading_fails_with_wrong_classname()); RUN_TEST(check_loading_fails_with_wrong_path()); RUN_TEST(check_loading_fails_plugin_is_not_owned_by_root()); + RUN_TEST(check_plugin_unload()); RUN_TEST(check_example_conversation_plugin_reason_log(false, "without_suspend")); RUN_TEST(check_example_conversation_plugin_reason_log(true, "with_suspend")); RUN_TEST(check_example_conversation_plugin_user_interrupts()); + RUN_TEST(check_plugin_unload()); RUN_TEST(check_example_policy_plugin_version_display(true)); RUN_TEST(check_example_policy_plugin_version_display(false)); @@ -1479,6 +1520,7 @@ main(int argc, char *argv[]) RUN_TEST(check_example_policy_plugin_validate_invalidate()); RUN_TEST(check_policy_plugin_callbacks_are_optional()); RUN_TEST(check_policy_plugin_reports_error()); + RUN_TEST(check_plugin_unload()); RUN_TEST(check_example_audit_plugin_receives_accept()); RUN_TEST(check_example_audit_plugin_receives_reject()); @@ -1487,6 +1529,7 @@ main(int argc, char *argv[]) RUN_TEST(check_example_audit_plugin_version_display()); RUN_TEST(check_audit_plugin_callbacks_are_optional()); RUN_TEST(check_audit_plugin_reports_error()); + RUN_TEST(check_plugin_unload()); // Monday, too early RUN_TEST(check_example_approval_plugin( @@ -1504,6 +1547,7 @@ main(int argc, char *argv[]) RUN_TEST(check_multiple_approval_plugin_and_arguments()); RUN_TEST(check_python_plugins_do_not_affect_each_other()); + RUN_TEST(check_plugin_unload()); RUN_TEST(check_example_debugging("plugin@err")); RUN_TEST(check_example_debugging("plugin@info")); @@ -1514,6 +1558,7 @@ main(int argc, char *argv[]) RUN_TEST(check_example_debugging("py_calls@diag")); RUN_TEST(check_example_debugging("py_calls@info")); RUN_TEST(check_example_debugging("plugin@err")); + RUN_TEST(check_plugin_unload()); return EXIT_SUCCESS; } diff --git a/plugins/python/regress/testdata/check_example_debugging_load@diag.log b/plugins/python/regress/testdata/check_example_debugging_load@diag.log index 5e2bece2f..15b4bbe51 100644 --- a/plugins/python/regress/testdata/check_example_debugging_load@diag.log +++ b/plugins/python/regress/testdata/check_example_debugging_load@diag.log @@ -1,4 +1,3 @@ importing module: SRC_DIR/example_debugging.py Extending python 'path' with 'SRC_DIR' -Closing: 0 python plugins left open -Closing: deinit python interpreter +Deinit was called for a python plugin diff --git a/plugins/python/regress/testhelpers.c b/plugins/python/regress/testhelpers.c index 7ce17b822..dd7f3bc97 100644 --- a/plugins/python/regress/testhelpers.c +++ b/plugins/python/regress/testhelpers.c @@ -262,7 +262,7 @@ mock_python_datetime_now(const char *plugin_name, const char *date_str) "import time\n" "from unittest.mock import Mock\n" "%s.datetime = Mock()\n" // replace plugin's datetime - "%s.datetime.now = lambda: datetime.fromtimestamp(time.mktime(time.strptime('%s', '%%Y-%%m-%%dT%%H:%%M:%%S')))\n", + "%s.datetime.now = lambda: datetime.strptime('%s', '%%Y-%%m-%%dT%%H:%%M:%%S')\n", plugin_name, plugin_name, plugin_name, date_str); VERIFY_PTR_NE(cmd, NULL); VERIFY_INT(PyRun_SimpleString(cmd), 0);