From 27de7dd24d0bd983b8254fe57b4a1aafb82c935c Mon Sep 17 00:00:00 2001 From: Robert Manner Date: Mon, 17 Feb 2020 16:16:04 +0100 Subject: [PATCH] plugins/python: only deinit interpreters when sudo unlinks the plugin This only happens when sudo unloads the last python plugin. The reason doing so is because there are some python modules which does not support importing them again after destroying the interpreter which has imported them previously. Another solution would be to just leak the interpreters (let the kernel free up), but then there might be some python resources like open files would not get cleaned up correctly if the plugin is badly written. Tests are meant to test the scenario sudo does, so I have modified them to generally do not unlink but only a few times (~per plugin type) so it does not use 48 interpreters (one gets started on every plugin->open) and it is visible at least which type of plugin fails deinit if there is an error. --- plugins/python/pyhelpers.c | 5 +- plugins/python/pyhelpers.h | 5 +- plugins/python/python_plugin_common.c | 83 +++++++++++++------ .../python/regress/check_python_examples.c | 73 ++++++++++++---- .../check_example_debugging_load@diag.log | 3 +- plugins/python/regress/testhelpers.c | 2 +- 6 files changed, 124 insertions(+), 47 deletions(-) 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);