From ce24b2c6e07c75bcc355a6ad4477b576c3155051 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 14 Jul 2026 15:05:36 +0200 Subject: [PATCH 1/9] go-exec: Run the prompts callbacks in the main thread pam_prompt leads to calling code into the caller application, and doing this in a different thread may be unsafe. So run a separated context/loop in the main thread and use it to call code that should run in the main thread --- pam/go-exec/module.c | 145 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 126 insertions(+), 19 deletions(-) diff --git a/pam/go-exec/module.c b/pam/go-exec/module.c index 63523e4a22..850d22b379 100644 --- a/pam/go-exec/module.c +++ b/pam/go-exec/module.c @@ -51,6 +51,7 @@ typedef struct typedef struct _ActionData { ModuleData *module_data; + GMainContext *action_context; GMainLoop *loop; GDBusConnection *connection; @@ -62,6 +63,10 @@ typedef struct _ActionData guint object_registered_id; guint log_handler_id; int log_file_fd; + +#ifdef AUTHD_TEST_MODULE + GThread *main_thread; +#endif } ActionData; const char *UBUNTU_AUTHD_PAM_OBJECT_NODE = @@ -308,13 +313,14 @@ action_module_data_cleanup (ActionData *action_data) g_log_set_debug_enabled (FALSE); g_clear_object (&action_data->cancellable); + g_clear_pointer (&action_data->action_context, g_main_context_unref); g_clear_pointer (&action_data->loop, g_main_loop_unref); g_clear_handle_id (&action_data->child_pid, g_spawn_close_pid); G_LOCK (logger); if (action_data->log_handler_id) g_log_remove_handler (G_LOG_DOMAIN, action_data->log_handler_id); -#if AUTHD_TEST_MODULE +#ifdef AUTHD_TEST_MODULE /* During tests we are catching catch all the domains! */ g_log_set_default_handler (g_log_default_handler, NULL); #endif @@ -405,9 +411,9 @@ is_debug_logging_enabled () typedef struct { - pid_t child_pid; - GMainLoop *main_loop; - GDBusConnection **connection_ptr; + pid_t child_pid; + GMainLoop *main_loop; + GDBusConnection **connection_ptr; } WaitChildThreadData; static gpointer @@ -482,6 +488,49 @@ sanitize_variant_key (const char *key) return g_strdup_printf ("exec-module-variant-%s", key); } +typedef struct +{ + ActionData *action_data; + GDBusMethodInvocation *invocation; + char *prompt; + int style; +} PromptInvocationData; + +static gboolean +invoke_prompt_on_main_thread (gpointer data) +{ + PromptInvocationData *prompt_data = data; + ActionData *action_data = prompt_data->action_data; + pam_handle_t *pamh = action_data->module_data->pamh; + g_autofree char *response = NULL; + int ret; + +#if AUTHD_TEST_MODULE + g_assert (action_data->main_thread == g_thread_self ()); + g_assert (g_main_context_is_owner (action_data->action_context)); +#endif + + ret = pam_prompt (pamh, prompt_data->style, + &response, "%s", + prompt_data->prompt); + + g_dbus_method_invocation_return_value (prompt_data->invocation, + g_variant_new ("(is)", ret, + response ? response : "")); + + return G_SOURCE_REMOVE; +} + +static void +prompt_invocation_data_free (gpointer data) +{ + PromptInvocationData *prompt_data = data; + + g_clear_object (&prompt_data->invocation); + g_clear_pointer (&prompt_data->prompt, g_free); + g_free (prompt_data); +} + static void on_pam_method_call (GDBusConnection *connection, const char *sender, @@ -648,17 +697,25 @@ on_pam_method_call (GDBusConnection *connection, } else if (g_str_equal (method_name, "Prompt")) { - g_autofree char *response = NULL; - const char *prompt; + PromptInvocationData *prompt_data = NULL; + g_autofree char *prompt = NULL; int style; - int ret; - - g_variant_get (parameters, "(i&s)", &style, &prompt); - ret = pam_prompt (pamh, style, &response, "%s", prompt); - g_dbus_method_invocation_return_value (invocation, - g_variant_new ("(is)", ret, - response ? response : "")); + g_variant_get (parameters, "(is)", &style, &prompt); + prompt_data = g_new0 (PromptInvocationData, 1); + + *prompt_data = (PromptInvocationData){ + .action_data = action_data, + .invocation = g_object_ref (invocation), + .style = style, + .prompt = g_steal_pointer (&prompt), + }; + + g_main_context_invoke_full (action_data->action_context, + G_PRIORITY_DEFAULT, + invoke_prompt_on_main_thread, + g_steal_pointer (&prompt_data), + prompt_invocation_data_free); } else { @@ -968,6 +1025,10 @@ static int do_pam_action_thread (pam_handle_t *pamh, ActionType action, int flags, + GMainContext *action_context, +#ifdef AUTHD_TEST_MODULE + GThread *main_thread, +#endif int argc, const char **argv) { @@ -1093,7 +1154,11 @@ do_pam_action_thread (pam_handle_t *pamh, g_atomic_pointer_compare_and_exchange (&module_data->server, NULL, g_object_ref (server)); action_data.module_data = module_data; + action_data.action_context = g_main_context_ref (action_context); action_data.cancellable = g_cancellable_new (); +#ifdef AUTHD_TEST_MODULE + action_data.main_thread = main_thread; +#endif main_context = g_main_context_ref (module_data->main_context); context_pusher = g_main_context_pusher_new (main_context); @@ -1229,15 +1294,41 @@ typedef struct int flags; int argc; const char **argv; +#ifdef AUTHD_TEST_MODULE + GThread *main_thread; +#endif + GMainContext *action_context; + GMainLoop *action_loop; } ActionThreadArgs; +static inline gboolean +quit_loop_source_callback (gpointer data) +{ + GMainLoop *action_loop = data; + + g_main_loop_quit (action_loop); + return G_SOURCE_REMOVE; +} + static inline gpointer do_pam_action_thread_adapter (gpointer data) { ActionThreadArgs * args = data; - return GINT_TO_POINTER (do_pam_action_thread (args->pamh, - args->action, args->flags, - args->argc, args->argv)); + int ret = do_pam_action_thread (args->pamh, + args->action, args->flags, + args->action_context, +#ifdef AUTHD_TEST_MODULE + args->main_thread, +#endif + args->argc, args->argv); + + g_main_context_invoke_full (args->action_context, + G_PRIORITY_DEFAULT, + quit_loop_source_callback, + g_main_loop_ref (args->action_loop), + (GDestroyNotify) g_main_loop_unref); + + return GINT_TO_POINTER (ret); } static inline int @@ -1247,6 +1338,8 @@ do_pam_action (pam_handle_t *pamh, int argc, const char **argv) { + g_autoptr(GMainContext) action_context = NULL; + g_autoptr(GMainLoop) action_loop = NULL; g_autoptr(GThread) thread = NULL; #ifndef AUTHD_TEST_EXEC_MODULE @@ -1260,19 +1353,33 @@ do_pam_action (pam_handle_t *pamh, case action_type_open_session: case action_type_close_session: return PAM_IGNORE; + default: break; } #endif - thread = g_thread_new (action_type_to_string (action), - do_pam_action_thread_adapter, &(ActionThreadArgs){ + action_context = g_main_context_new (); + action_loop = g_main_loop_new (action_context, FALSE); + + ActionThreadArgs thread_args = { .pamh = pamh, .action = action, .flags = flags, + #ifdef AUTHD_TEST_MODULE + .main_thread = g_thread_self (), + #endif .argc = argc, .argv = argv, - }); + .action_context = action_context, + .action_loop = action_loop, + }; + + thread = g_thread_new (action_type_to_string (action), + do_pam_action_thread_adapter, &thread_args); + + g_main_loop_run (action_loop); + return GPOINTER_TO_INT (g_thread_join (g_steal_pointer (&thread))); } From 705b7a0991ec4af8279255113c13abaea7dae37f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 15 Jul 2026 10:52:20 +0200 Subject: [PATCH 2/9] go-exec: Move pam handle into ActionData While it's a per-module information there's no need to save it there since it's something that will be available for each action, and this would allow further simplifications --- pam/go-exec/module.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/pam/go-exec/module.c b/pam/go-exec/module.c index 850d22b379..34a8d4c849 100644 --- a/pam/go-exec/module.c +++ b/pam/go-exec/module.c @@ -39,7 +39,6 @@ typedef enum _ActionType { typedef struct { /* Per module-instance data */ - pam_handle_t *pamh; GDBusServer *server; GMainContext *main_context; GCancellable *cancellable; @@ -50,6 +49,7 @@ typedef struct /* Per action data, protected by the static mutex */ typedef struct _ActionData { + pam_handle_t *pamh; ModuleData *module_data; GMainContext *action_context; @@ -388,7 +388,6 @@ setup_shared_module_data (pam_handle_t *pamh) return NULL; } - module_data->pamh = pamh; module_data->cancellable = g_cancellable_new (); return module_data; @@ -501,7 +500,6 @@ invoke_prompt_on_main_thread (gpointer data) { PromptInvocationData *prompt_data = data; ActionData *action_data = prompt_data->action_data; - pam_handle_t *pamh = action_data->module_data->pamh; g_autofree char *response = NULL; int ret; @@ -510,7 +508,8 @@ invoke_prompt_on_main_thread (gpointer data) g_assert (g_main_context_is_owner (action_data->action_context)); #endif - ret = pam_prompt (pamh, prompt_data->style, + ret = pam_prompt (action_data->pamh, + prompt_data->style, &response, "%s", prompt_data->prompt); @@ -542,7 +541,7 @@ on_pam_method_call (GDBusConnection *connection, void *user_data) { ActionData *action_data = user_data; - pam_handle_t *pamh = action_data->module_data->pamh; + pam_handle_t *pamh = action_data->pamh; if (is_debug_logging_enabled ()) { @@ -763,7 +762,7 @@ on_new_connection (G_GNUC_UNUSED GDBusServer *server, g_autoptr(GDBusNodeInfo) node = NULL; g_autoptr(GError) error = NULL; ActionData *action_data = user_data; - pam_handle_t *pamh = action_data->module_data->pamh; + pam_handle_t *pamh = action_data->pamh; GCredentials *credentials; pid_t client_pid; @@ -851,8 +850,9 @@ on_new_connection (G_GNUC_UNUSED GDBusServer *server, } static GDBusServer * -setup_dbus_server (ModuleData *module_data, - GError **error) +setup_dbus_server (pam_handle_t *pamh, + ModuleData *module_data, + GError **error) { GDBusServer *server = NULL; g_autoptr(GMainContextPusher) context_pusher G_GNUC_UNUSED = NULL; @@ -877,7 +877,7 @@ setup_dbus_server (ModuleData *module_data, context_pusher = g_main_context_pusher_new (main_context); - pam_get_item (module_data->pamh, PAM_SERVICE, (const void **) &service_name); + pam_get_item (pamh, PAM_SERVICE, (const void **) &service_name); guid = g_dbus_generate_guid (); server_addr = g_strdup_printf ("unix:abstract=authd-%s-%s", service_name, guid); @@ -1034,7 +1034,11 @@ do_pam_action_thread (pam_handle_t *pamh, { ModuleData *module_data = NULL; g_autoptr(GMutexLocker) G_GNUC_UNUSED locker = NULL; - g_auto(ActionData) action_data = {.current_action = action, 0}; + g_auto(ActionData) action_data = { + .current_action = action, + .pamh = pamh, + 0 + }; g_autoptr(GMainContextPusher) context_pusher G_GNUC_UNUSED = NULL; g_autoptr(GMainContext) main_context = NULL; g_autoptr(GError) error = NULL; @@ -1143,7 +1147,7 @@ do_pam_action_thread (pam_handle_t *pamh, return PAM_MODULE_UNKNOWN; } - server = setup_dbus_server (module_data, &error); + server = setup_dbus_server (pamh, module_data, &error); if (!server) { notify_error (pamh, action, "can't create D-Bus connection: %s", error->message); From 34d70b0ca2a581aa11d03aec759f9668f29a242e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 15 Jul 2026 11:47:37 +0200 Subject: [PATCH 3/9] go-exec: Use ActionData as parameter of notify error We may want to notify this in the main thread too, so let's prepare the code for that --- pam/go-exec/module.c | 49 ++++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/pam/go-exec/module.c b/pam/go-exec/module.c index 34a8d4c849..da7d4cb0c7 100644 --- a/pam/go-exec/module.c +++ b/pam/go-exec/module.c @@ -190,29 +190,31 @@ action_type_to_string (ActionType action_type) g_return_val_if_reached ("unknown"); } -G_GNUC_PRINTF (3, 4) +G_GNUC_PRINTF (2, 3) static void -notify_error (pam_handle_t *pamh, - ActionType action_type, - const char *format, +notify_error (ActionData *action_data, + const char *format, ...) { - const char *action = action_type_to_string (action_type); g_autofree char *message = NULL; + const char *action; va_list args; + g_return_if_fail (action_data != NULL); g_return_if_fail (format != NULL); va_start (args, format); message = g_strdup_vprintf (format, args); va_end (args); + action = action_type_to_string (action_data->current_action); + if (isatty (STDERR_FILENO)) \ g_debug ("%s: %s", action, message); else g_warning ("%s: %s", action, message); - pam_error (pamh, "%s: %s", action, message); + pam_error (action_data->pamh, "%s: %s", action, message); } static GLogWriterOutput @@ -762,7 +764,6 @@ on_new_connection (G_GNUC_UNUSED GDBusServer *server, g_autoptr(GDBusNodeInfo) node = NULL; g_autoptr(GError) error = NULL; ActionData *action_data = user_data; - pam_handle_t *pamh = action_data->pamh; GCredentials *credentials; pid_t client_pid; @@ -770,21 +771,21 @@ on_new_connection (G_GNUC_UNUSED GDBusServer *server, if (action_data->connection) { - notify_error (pamh, action_data->current_action, + notify_error (action_data, "Another client is already using this connection"); return FALSE; } if (!G_IS_CREDENTIALS (credentials)) { - notify_error (pamh, action_data->current_action, + notify_error (action_data, "Impossible to get credentials, refusing the connection..."); return FALSE; } if ((client_pid = g_credentials_get_unix_pid (credentials, &error)) == -1) { - notify_error (pamh, action_data->current_action, + notify_error (action_data, "Impossible to get client PID (%s), refusing the connection...", error->message); return FALSE; @@ -801,7 +802,7 @@ on_new_connection (G_GNUC_UNUSED GDBusServer *server, { const char *test_name; - test_name = pam_getenv (pamh, "AUTHD_PAM_CLI_TEST_NAME"); + test_name = pam_getenv (action_data->pamh, "AUTHD_PAM_CLI_TEST_NAME"); g_debug ("%s: Client pid %d does not match with expected %d", test_name, client_pid, action_data->child_pid); @@ -812,7 +813,7 @@ on_new_connection (G_GNUC_UNUSED GDBusServer *server, if (client_pid != action_data->child_pid && client_pid != getpid ()) { - notify_error (pamh, action_data->current_action, + notify_error (action_data, "Child PID is not matching the expected one"); return FALSE; } @@ -820,7 +821,7 @@ on_new_connection (G_GNUC_UNUSED GDBusServer *server, node = g_dbus_node_info_new_for_xml (UBUNTU_AUTHD_PAM_OBJECT_NODE, &error); if (!node) { - notify_error (pamh, action_data->current_action, + notify_error (action_data, "Can't create node: %s", error->message); return FALSE; } @@ -1082,7 +1083,7 @@ do_pam_action_thread (pam_handle_t *pamh, if (!handle_module_options (argc, argv, &args, &env_variables, &log_file, &error)) { G_UNLOCK (logger); - notify_error (pamh, action, "impossible to parse arguments: %s", error->message); + notify_error (&action_data, "impossible to parse arguments: %s", error->message); return PAM_SYSTEM_ERR; } @@ -1123,13 +1124,13 @@ do_pam_action_thread (pam_handle_t *pamh, module_data = setup_shared_module_data (pamh); if (module_data == NULL) { - notify_error (pamh, action, "can't create module data"); + notify_error (&action_data, "can't create module data"); return PAM_SYSTEM_ERR; } if (!args || args->len < 1) { - notify_error (pamh, action, "no executable provided"); + notify_error (&action_data, "no executable provided"); return PAM_MODULE_UNKNOWN; } @@ -1137,20 +1138,20 @@ do_pam_action_thread (pam_handle_t *pamh, if (!exe || *exe == '\0') { - notify_error (pamh, action, "no valid module name provided"); + notify_error (&action_data, "no valid module name provided"); return PAM_MODULE_UNKNOWN; } if (!g_file_test (exe, G_FILE_TEST_IS_EXECUTABLE)) { - notify_error (pamh, action, "Impossible to use %s as PAM executable", exe); + notify_error (&action_data, "Impossible to use %s as PAM executable", exe); return PAM_MODULE_UNKNOWN; } server = setup_dbus_server (pamh, module_data, &error); if (!server) { - notify_error (pamh, action, "can't create D-Bus connection: %s", error->message); + notify_error (&action_data, "can't create D-Bus connection: %s", error->message); return PAM_SYSTEM_ERR; } @@ -1173,21 +1174,21 @@ do_pam_action_thread (pam_handle_t *pamh, { if ((stdin_fd = dup_fd_checked (STDIN_FILENO, &error)) < 0) { - notify_error (pamh, action, "can't duplicate stdin file descriptor: %s", + notify_error (&action_data, "can't duplicate stdin file descriptor: %s", error->message); return PAM_SYSTEM_ERR; } if ((stdout_fd = dup_fd_checked (STDOUT_FILENO, &error)) < 0) { - notify_error (pamh, action, "can't duplicate stdout file descriptor: %s", + notify_error (&action_data, "can't duplicate stdout file descriptor: %s", error->message); return PAM_SYSTEM_ERR; } if ((stderr_fd = dup_fd_checked (STDERR_FILENO, &error)) < 0) { - notify_error (pamh, action, "can't duplicate stderr file descriptor: %s", + notify_error (&action_data, "can't duplicate stderr file descriptor: %s", error->message); return PAM_SYSTEM_ERR; } @@ -1255,7 +1256,7 @@ do_pam_action_thread (pam_handle_t *pamh, stderr_fd, &error)) { - notify_error (pamh, action, "can't launch %s: %s", exe, error->message); + notify_error (&action_data, "can't launch %s: %s", exe, error->message); return PAM_SYSTEM_ERR; } @@ -1276,7 +1277,7 @@ do_pam_action_thread (pam_handle_t *pamh, if (exit_status < 0) { - notify_error (pamh, action, "Waiting for PID %" G_PID_FORMAT + notify_error (&action_data, "Waiting for PID %" G_PID_FORMAT " failed with error %s", child_pid, g_strerror (-exit_status)); exit_status = PAM_SYSTEM_ERR; From 970ac66fbe26de35e17e188d5c2b4817661d2ca8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 15 Jul 2026 11:52:18 +0200 Subject: [PATCH 4/9] go-exec: Also notify errors in the same thread of the calling app pam_error implies calling conversation functions in the calling application, so do it in the same thread. --- pam/go-exec/module.c | 75 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 67 insertions(+), 8 deletions(-) diff --git a/pam/go-exec/module.c b/pam/go-exec/module.c index da7d4cb0c7..ef85230c29 100644 --- a/pam/go-exec/module.c +++ b/pam/go-exec/module.c @@ -190,6 +190,44 @@ action_type_to_string (ActionType action_type) g_return_val_if_reached ("unknown"); } +typedef struct +{ + ActionData *action_data; + char *message; + + GMutex completion_mutex; + GCond completion_cond; + gboolean completed; +} PamErrorInvocationData; + +static gboolean +invoke_pam_error_on_action_context (gpointer data) +{ + PamErrorInvocationData *invocation = data; + ActionData *action_data = invocation->action_data; + +#if AUTHD_TEST_MODULE + g_assert (action_data->main_thread == g_thread_self ()); +#endif + + pam_error (action_data->pamh, "%s", invocation->message); + + g_mutex_lock (&invocation->completion_mutex); + invocation->completed = TRUE; + g_cond_signal (&invocation->completion_cond); + g_mutex_unlock (&invocation->completion_mutex); + + return G_SOURCE_REMOVE; +} + +static void +pam_error_invocation_data_clear (gpointer data) +{ + PamErrorInvocationData *invocation = data; + + g_clear_pointer (&invocation->message, g_free); +} + G_GNUC_PRINTF (2, 3) static void notify_error (ActionData *action_data, @@ -214,7 +252,29 @@ notify_error (ActionData *action_data, else g_warning ("%s: %s", action, message); - pam_error (action_data->pamh, "%s: %s", action, message); + PamErrorInvocationData invocation = { + .action_data = action_data, + .message = g_strdup_printf ("%s: %s", action, message), + .completed = FALSE, + }; + + g_mutex_init (&invocation.completion_mutex); + g_cond_init (&invocation.completion_cond); + + g_assert (action_data->action_context); + g_main_context_invoke_full (action_data->action_context, + G_PRIORITY_DEFAULT, + invoke_pam_error_on_action_context, + &invocation, + pam_error_invocation_data_clear); + + g_mutex_lock (&invocation.completion_mutex); + while (!invocation.completed) + g_cond_wait (&invocation.completion_cond, &invocation.completion_mutex); + g_mutex_unlock (&invocation.completion_mutex); + + g_cond_clear (&invocation.completion_cond); + g_mutex_clear (&invocation.completion_mutex); } static GLogWriterOutput @@ -505,7 +565,7 @@ invoke_prompt_on_main_thread (gpointer data) g_autofree char *response = NULL; int ret; -#if AUTHD_TEST_MODULE +#ifdef AUTHD_TEST_MODULE g_assert (action_data->main_thread == g_thread_self ()); g_assert (g_main_context_is_owner (action_data->action_context)); #endif @@ -1036,9 +1096,12 @@ do_pam_action_thread (pam_handle_t *pamh, ModuleData *module_data = NULL; g_autoptr(GMutexLocker) G_GNUC_UNUSED locker = NULL; g_auto(ActionData) action_data = { - .current_action = action, .pamh = pamh, - 0 + .current_action = action, + .action_context = g_main_context_ref (action_context), +#ifdef AUTHD_TEST_MODULE + .main_thread = main_thread, +#endif }; g_autoptr(GMainContextPusher) context_pusher G_GNUC_UNUSED = NULL; g_autoptr(GMainContext) main_context = NULL; @@ -1159,11 +1222,7 @@ do_pam_action_thread (pam_handle_t *pamh, g_atomic_pointer_compare_and_exchange (&module_data->server, NULL, g_object_ref (server)); action_data.module_data = module_data; - action_data.action_context = g_main_context_ref (action_context); action_data.cancellable = g_cancellable_new (); -#ifdef AUTHD_TEST_MODULE - action_data.main_thread = main_thread; -#endif main_context = g_main_context_ref (module_data->main_context); context_pusher = g_main_context_pusher_new (main_context); From 0bb250bd1c5365e3cf7fbed29776226b0907669b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 16 Jul 2026 00:38:26 +0200 Subject: [PATCH 5/9] go-exec: Return invocation on the action thread While this is not fully required because g_dbus_method_invocation_return_value() is thread safe in this context, it's still better to use it in the threads it belongs. --- pam/go-exec/module.c | 46 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/pam/go-exec/module.c b/pam/go-exec/module.c index ef85230c29..e2a2293c12 100644 --- a/pam/go-exec/module.c +++ b/pam/go-exec/module.c @@ -557,11 +557,44 @@ typedef struct int style; } PromptInvocationData; +typedef struct +{ + GDBusMethodInvocation *invocation; + gchar *response; + int ret; +} ReturnInvocationData; + +static void +return_invocation_data_free (gpointer data) +{ + ReturnInvocationData *return_data = data; + + g_clear_object (&return_data->invocation); + g_clear_pointer (&return_data->response, g_free); + g_free (return_data); +} + +G_DEFINE_AUTOPTR_CLEANUP_FUNC (ReturnInvocationData, return_invocation_data_free) + +static gboolean +return_invocation_on_main_thread (gpointer data) +{ + ReturnInvocationData *return_data = data; + + g_dbus_method_invocation_return_value (return_data->invocation, + g_variant_new ("(is)", + return_data->ret, + return_data->response)); + + return G_SOURCE_REMOVE; +} + static gboolean invoke_prompt_on_main_thread (gpointer data) { PromptInvocationData *prompt_data = data; ActionData *action_data = prompt_data->action_data; + g_autoptr(ReturnInvocationData) return_data = NULL; g_autofree char *response = NULL; int ret; @@ -575,9 +608,16 @@ invoke_prompt_on_main_thread (gpointer data) &response, "%s", prompt_data->prompt); - g_dbus_method_invocation_return_value (prompt_data->invocation, - g_variant_new ("(is)", ret, - response ? response : "")); + return_data = g_new0 (ReturnInvocationData, 1); + return_data->ret = ret; + return_data->invocation = g_object_ref (prompt_data->invocation); + return_data->response = g_steal_pointer (&response); + + g_main_context_invoke_full (action_data->module_data->main_context, + G_PRIORITY_DEFAULT, + return_invocation_on_main_thread, + g_steal_pointer (&return_data), + return_invocation_data_free); return G_SOURCE_REMOVE; } From b5544844fba5140b0deabe70f322be6d69a3f477 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 16 Jul 2026 00:42:58 +0200 Subject: [PATCH 6/9] go-exec: Do not leak a variant on pam_set_data() failure --- pam/go-exec/module.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pam/go-exec/module.c b/pam/go-exec/module.c index e2a2293c12..3d677c3c2f 100644 --- a/pam/go-exec/module.c +++ b/pam/go-exec/module.c @@ -757,6 +757,9 @@ on_pam_method_call (GDBusConnection *connection, variant_key = sanitize_variant_key (key); ret = pam_set_data (pamh, variant_key, variant, on_variant_data_removed); g_dbus_method_invocation_return_value (invocation, g_variant_new ("(i)", ret)); + + if (ret != PAM_SUCCESS) + g_clear_pointer (&variant, g_variant_unref); } else if (g_str_equal (method_name, "UnsetData")) { From 3fb5b25a97e8e4a6fc6a35fb7202823ca6c597a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 16 Jul 2026 01:10:34 +0200 Subject: [PATCH 7/9] go-exec: Unify conversation code handling to custom helpers As pam_error() is just a definition for pam_prompt(), we can unify the code to run them in the main action thread. Add an helper to use conversations from the model in the main thread and share the same code for all the conversations we trigger. --- pam/go-exec/module.c | 169 ++++++++++++++++++++++++------------------- 1 file changed, 93 insertions(+), 76 deletions(-) diff --git a/pam/go-exec/module.c b/pam/go-exec/module.c index 3d677c3c2f..b5af96da1b 100644 --- a/pam/go-exec/module.c +++ b/pam/go-exec/module.c @@ -190,42 +190,85 @@ action_type_to_string (ActionType action_type) g_return_val_if_reached ("unknown"); } -typedef struct +typedef struct _ConversationInvocationData ConversationInvocationData; +typedef void (*ConversationDoneCallback) (ConversationInvocationData *invocation); + +struct _ConversationInvocationData { - ActionData *action_data; - char *message; + ActionData *action_data; + char *message; + int style; + int ret; + char *response; + ConversationDoneCallback done_callback; + gpointer callback_data; + GDestroyNotify callback_data_destroy; +}; + +static void +conversation_invocation_data_free (gpointer data) +{ + ConversationInvocationData *invocation = data; + + g_clear_pointer (&invocation->message, g_free); + g_clear_pointer (&invocation->response, g_free); + + if (invocation->callback_data_destroy) + { + g_clear_pointer (&invocation->callback_data, + invocation->callback_data_destroy); + } - GMutex completion_mutex; - GCond completion_cond; - gboolean completed; -} PamErrorInvocationData; + g_free (invocation); +} + +G_DEFINE_AUTOPTR_CLEANUP_FUNC (ConversationInvocationData, + conversation_invocation_data_free); static gboolean -invoke_pam_error_on_action_context (gpointer data) +invoke_conversation_on_action_context (gpointer data) { - PamErrorInvocationData *invocation = data; + ConversationInvocationData *invocation = data; ActionData *action_data = invocation->action_data; + char **response_ptr = NULL; #if AUTHD_TEST_MODULE g_assert (action_data->main_thread == g_thread_self ()); + g_assert (g_main_context_is_owner (action_data->action_context)); #endif - pam_error (action_data->pamh, "%s", invocation->message); + if (invocation->style == PAM_PROMPT_ECHO_ON || + invocation->style == PAM_PROMPT_ECHO_OFF) + response_ptr = &invocation->response; - g_mutex_lock (&invocation->completion_mutex); - invocation->completed = TRUE; - g_cond_signal (&invocation->completion_cond); - g_mutex_unlock (&invocation->completion_mutex); + invocation->ret = pam_prompt (action_data->pamh, + invocation->style, + response_ptr, + "%s", + invocation->message); + + if (invocation->done_callback) + invocation->done_callback (invocation); return G_SOURCE_REMOVE; } +typedef struct +{ + GMutex completion_mutex; + GCond completion_cond; + gboolean completed; +} NotifyConversationData; + static void -pam_error_invocation_data_clear (gpointer data) +notify_conversation_done (ConversationInvocationData *invocation) { - PamErrorInvocationData *invocation = data; + NotifyConversationData *notify_data = invocation->callback_data; - g_clear_pointer (&invocation->message, g_free); + g_mutex_lock (¬ify_data->completion_mutex); + notify_data->completed = TRUE; + g_cond_signal (¬ify_data->completion_cond); + g_mutex_unlock (¬ify_data->completion_mutex); } G_GNUC_PRINTF (2, 3) @@ -235,6 +278,8 @@ notify_error (ActionData *action_data, ...) { g_autofree char *message = NULL; + g_autoptr(ConversationInvocationData) invocation = NULL; + NotifyConversationData notify_data = { .completed = FALSE }; const char *action; va_list args; @@ -252,29 +297,32 @@ notify_error (ActionData *action_data, else g_warning ("%s: %s", action, message); - PamErrorInvocationData invocation = { + invocation = g_new0 (ConversationInvocationData, 1); + *invocation = (ConversationInvocationData){ .action_data = action_data, + .style = PAM_ERROR_MSG, .message = g_strdup_printf ("%s: %s", action, message), - .completed = FALSE, + .done_callback = notify_conversation_done, + .callback_data = ¬ify_data, }; - g_mutex_init (&invocation.completion_mutex); - g_cond_init (&invocation.completion_cond); + g_mutex_init (¬ify_data.completion_mutex); + g_cond_init (¬ify_data.completion_cond); g_assert (action_data->action_context); g_main_context_invoke_full (action_data->action_context, G_PRIORITY_DEFAULT, - invoke_pam_error_on_action_context, - &invocation, - pam_error_invocation_data_clear); + invoke_conversation_on_action_context, + g_steal_pointer (&invocation), + conversation_invocation_data_free); - g_mutex_lock (&invocation.completion_mutex); - while (!invocation.completed) - g_cond_wait (&invocation.completion_cond, &invocation.completion_mutex); - g_mutex_unlock (&invocation.completion_mutex); + g_mutex_lock (¬ify_data.completion_mutex); + while (!notify_data.completed) + g_cond_wait (¬ify_data.completion_cond, ¬ify_data.completion_mutex); + g_mutex_unlock (¬ify_data.completion_mutex); - g_cond_clear (&invocation.completion_cond); - g_mutex_clear (&invocation.completion_mutex); + g_cond_clear (¬ify_data.completion_cond); + g_mutex_clear (¬ify_data.completion_mutex); } static GLogWriterOutput @@ -549,14 +597,6 @@ sanitize_variant_key (const char *key) return g_strdup_printf ("exec-module-variant-%s", key); } -typedef struct -{ - ActionData *action_data; - GDBusMethodInvocation *invocation; - char *prompt; - int style; -} PromptInvocationData; - typedef struct { GDBusMethodInvocation *invocation; @@ -589,47 +629,22 @@ return_invocation_on_main_thread (gpointer data) return G_SOURCE_REMOVE; } -static gboolean -invoke_prompt_on_main_thread (gpointer data) +static void +conversation_prompt_done (ConversationInvocationData *invocation) { - PromptInvocationData *prompt_data = data; - ActionData *action_data = prompt_data->action_data; + ActionData *action_data = invocation->action_data; g_autoptr(ReturnInvocationData) return_data = NULL; - g_autofree char *response = NULL; - int ret; - -#ifdef AUTHD_TEST_MODULE - g_assert (action_data->main_thread == g_thread_self ()); - g_assert (g_main_context_is_owner (action_data->action_context)); -#endif - - ret = pam_prompt (action_data->pamh, - prompt_data->style, - &response, "%s", - prompt_data->prompt); return_data = g_new0 (ReturnInvocationData, 1); - return_data->ret = ret; - return_data->invocation = g_object_ref (prompt_data->invocation); - return_data->response = g_steal_pointer (&response); + return_data->ret = invocation->ret; + return_data->invocation = g_object_ref (invocation->callback_data); + return_data->response = g_strdup (invocation->response ? invocation->response : ""); g_main_context_invoke_full (action_data->module_data->main_context, G_PRIORITY_DEFAULT, return_invocation_on_main_thread, g_steal_pointer (&return_data), return_invocation_data_free); - - return G_SOURCE_REMOVE; -} - -static void -prompt_invocation_data_free (gpointer data) -{ - PromptInvocationData *prompt_data = data; - - g_clear_object (&prompt_data->invocation); - g_clear_pointer (&prompt_data->prompt, g_free); - g_free (prompt_data); } static void @@ -801,25 +816,27 @@ on_pam_method_call (GDBusConnection *connection, } else if (g_str_equal (method_name, "Prompt")) { - PromptInvocationData *prompt_data = NULL; + ConversationInvocationData *prompt_data = NULL; g_autofree char *prompt = NULL; int style; g_variant_get (parameters, "(is)", &style, &prompt); - prompt_data = g_new0 (PromptInvocationData, 1); + prompt_data = g_new0 (ConversationInvocationData, 1); - *prompt_data = (PromptInvocationData){ + *prompt_data = (ConversationInvocationData){ .action_data = action_data, - .invocation = g_object_ref (invocation), .style = style, - .prompt = g_steal_pointer (&prompt), + .message = g_steal_pointer (&prompt), + .done_callback = conversation_prompt_done, + .callback_data = g_object_ref (invocation), + .callback_data_destroy = (GDestroyNotify) g_object_unref, }; g_main_context_invoke_full (action_data->action_context, G_PRIORITY_DEFAULT, - invoke_prompt_on_main_thread, + invoke_conversation_on_action_context, g_steal_pointer (&prompt_data), - prompt_invocation_data_free); + conversation_invocation_data_free); } else { From 4041a9c3e5bb789e2c84a42b13f8f87f391b6a79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 16 Jul 2026 01:52:02 +0200 Subject: [PATCH 8/9] go-exec: Use GLib utility to check if debug logging is enabled --- pam/go-exec/module.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/pam/go-exec/module.c b/pam/go-exec/module.c index b5af96da1b..0b56e00b94 100644 --- a/pam/go-exec/module.c +++ b/pam/go-exec/module.c @@ -503,19 +503,10 @@ setup_shared_module_data (pam_handle_t *pamh) return module_data; } -static gboolean +static inline gboolean is_debug_logging_enabled () { - const char *debug_messages; - - if (g_log_get_debug_enabled ()) - return TRUE; - - if (!(debug_messages = g_getenv ("G_MESSAGES_DEBUG"))) - return FALSE; - - return g_str_equal (debug_messages, "all") || - strstr (debug_messages, G_LOG_DOMAIN); + return !g_log_writer_default_would_drop (G_LOG_LEVEL_DEBUG, G_LOG_DOMAIN); } typedef struct From 1ecdbdb6c095dd7e043f42a5be2f7033c946f444 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 16 Jul 2026 02:42:50 +0200 Subject: [PATCH 9/9] pam/integration-tests/exec: Only mark the test flaky on s390x Ideally the last changes should make the conversation handling safer and they seem to fix the issues when running in a local s390x container, but we may still get failures. So let's just skip the tests for the arch that seems to be problematic Related to: #966 --- pam/integration-tests/exec_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pam/integration-tests/exec_test.go b/pam/integration-tests/exec_test.go index f6a334d278..bd6afdc57b 100644 --- a/pam/integration-tests/exec_test.go +++ b/pam/integration-tests/exec_test.go @@ -26,9 +26,9 @@ const execServiceName = "exec-module" func TestExecModule(t *testing.T) { t.Parallel() - // This test is flaky, see https://github.com/canonical/authd/issues/966 - if os.Getenv("AUTHD_SKIP_FLAKY_TESTS") != "" { - t.Skip("skipping flaky test") + // This test is flaky in s390x, see https://github.com/canonical/authd/issues/966 + if runtime.GOARCH == "s390x" { + t.Skip("skipping on s390x due to known instability") } t.Cleanup(pam_test.MaybeDoLeakCheck)