From 1b58ba768ab9351fb3a0face3ebef9646157c4e4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 19 Nov 2024 15:55:03 +0100 Subject: [PATCH 1/4] logind: normalize parameter to create_session() We can pass a properly typed Manager object here, no reason to pass it as void*. --- src/login/logind-dbus.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 4b14b9b690..d5038812d8 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -85,6 +85,8 @@ static int get_sender_session( const char *name; int r; + assert(m); + /* Acquire the sender's session. This first checks if the sending process is inside a session itself, * and returns that. If not and 'consult_display' is true, this returns the display session of the * owning user of the caller. */ @@ -828,8 +830,8 @@ static int method_list_inhibitors(sd_bus_message *message, void *userdata, sd_bu } static int create_session( + Manager *m, sd_bus_message *message, - void *userdata, sd_bus_error *error, uid_t uid, pid_t leader_pid, @@ -848,7 +850,6 @@ static int create_session( uint64_t flags) { _cleanup_(pidref_done) PidRef leader = PIDREF_NULL; - Manager *m = ASSERT_PTR(userdata); _cleanup_free_ char *id = NULL; Session *session = NULL; uint32_t audit_id = 0; @@ -858,6 +859,7 @@ static int create_session( SessionClass c; int r; + assert(m); assert(message); if (!uid_is_valid(uid)) @@ -1176,8 +1178,8 @@ static int method_create_session(sd_bus_message *message, void *userdata, sd_bus return r; return create_session( - message, userdata, + message, error, uid, leader_pid, @@ -1224,8 +1226,8 @@ static int method_create_session_pidfd(sd_bus_message *message, void *userdata, return r; return create_session( - message, userdata, + message, error, uid, /* leader_pid = */ 0, From e79502cdfa948319a2d46cdfde1f2776d27bf428 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 19 Nov 2024 20:46:55 +0100 Subject: [PATCH 2/4] logind: split out logic that finds free session ID into helper call Just some refactoring to make an overly large function a bit smaller. --- src/login/logind-dbus.c | 80 +++++++++++++++++++++++++---------------- 1 file changed, 50 insertions(+), 30 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index d5038812d8..dc5c054d47 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -829,6 +829,53 @@ static int method_list_inhibitors(sd_bus_message *message, void *userdata, sd_bu return sd_bus_send(NULL, reply, NULL); } +static int manager_choose_session_id( + Manager *m, + const PidRef *leader, + char **ret_id) { + + int r; + + assert(m); + assert(pidref_is_set(leader)); + assert(ret_id); + + /* Try to keep our session IDs and the audit session IDs in sync */ + _cleanup_free_ char *id = NULL; + uint32_t audit_id = AUDIT_SESSION_INVALID; + r = audit_session_from_pid(leader, &audit_id); + if (r < 0) { + if (r != -ENODATA) + log_debug_errno(r, "Failed to read audit session ID of process " PID_FMT ", ignoring: %m", leader->pid); + } else { + if (asprintf(&id, "%"PRIu32, audit_id) < 0) + return -ENOMEM; + + /* Wut? There's already a session by this name and we didn't find it above? Weird, then let's + * not trust the audit data and let's better register a new ID */ + if (hashmap_contains(m->sessions, id)) { + log_warning("Existing logind session ID %s used by new audit session, ignoring.", id); + id = mfree(id); + } + } + + if (!id) + do { + id = mfree(id); + + if (asprintf(&id, "c%" PRIu64, ++m->session_counter) < 0) + return -ENOMEM; + + } while (hashmap_contains(m->sessions, id)); + + /* The generated names should not clash with 'auto' or 'self' */ + assert(!SESSION_IS_SELF(id)); + assert(!SESSION_IS_AUTO(id)); + + *ret_id = TAKE_PTR(id); + return 0; +} + static int create_session( Manager *m, sd_bus_message *message, @@ -852,7 +899,6 @@ static int create_session( _cleanup_(pidref_done) PidRef leader = PIDREF_NULL; _cleanup_free_ char *id = NULL; Session *session = NULL; - uint32_t audit_id = 0; User *user = NULL; Seat *seat = NULL; SessionType t; @@ -1008,35 +1054,9 @@ static int create_session( "Maximum number of sessions (%" PRIu64 ") reached, refusing further sessions.", m->sessions_max); - (void) audit_session_from_pid(&leader, &audit_id); - if (audit_session_is_valid(audit_id)) { - /* Keep our session IDs and the audit session IDs in sync */ - - if (asprintf(&id, "%"PRIu32, audit_id) < 0) - return -ENOMEM; - - /* Wut? There's already a session by this name and we didn't find it above? Weird, then let's - * not trust the audit data and let's better register a new ID */ - if (hashmap_contains(m->sessions, id)) { - log_warning("Existing logind session ID %s used by new audit session, ignoring.", id); - audit_id = AUDIT_SESSION_INVALID; - id = mfree(id); - } - } - - if (!id) { - do { - id = mfree(id); - - if (asprintf(&id, "c%" PRIu64, ++m->session_counter) < 0) - return -ENOMEM; - - } while (hashmap_contains(m->sessions, id)); - } - - /* The generated names should not clash with 'auto' or 'self' */ - assert(!SESSION_IS_SELF(id)); - assert(!SESSION_IS_AUTO(id)); + r = manager_choose_session_id(m, &leader, &id); + if (r < 0) + return r; /* If we are not watching utmp already, try again */ manager_reconnect_utmp(m); From 1b48aa35235bfbd005f8b9e6502a5beec2c77c64 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 19 Nov 2024 22:42:26 +0100 Subject: [PATCH 3/4] logind: indicate that 'error' parameter is input by making it const --- src/login/logind-session-dbus.c | 4 ++-- src/login/logind-session-dbus.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c index 5300284e5a..11c315ade7 100644 --- a/src/login/logind-session-dbus.c +++ b/src/login/logind-session-dbus.c @@ -912,7 +912,7 @@ static bool session_job_pending(Session *s) { (SESSION_CLASS_WANTS_SERVICE_MANAGER(s->class) && s->user->service_manager_job); } -int session_send_create_reply(Session *s, sd_bus_error *error) { +int session_send_create_reply(Session *s, const sd_bus_error *error) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *c = NULL; _cleanup_close_ int fifo_fd = -EBADF; _cleanup_free_ char *p = NULL; @@ -968,7 +968,7 @@ int session_send_create_reply(Session *s, sd_bus_error *error) { false); } -int session_send_upgrade_reply(Session *s, sd_bus_error *error) { +int session_send_upgrade_reply(Session *s, const sd_bus_error *error) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *c = NULL; assert(s); diff --git a/src/login/logind-session-dbus.h b/src/login/logind-session-dbus.h index 37a107ec33..2ff686dbd2 100644 --- a/src/login/logind-session-dbus.h +++ b/src/login/logind-session-dbus.h @@ -15,8 +15,8 @@ int session_send_changed(Session *s, const char *properties, ...) _sentinel_; int session_send_lock(Session *s, bool lock); int session_send_lock_all(Manager *m, bool lock); -int session_send_create_reply(Session *s, sd_bus_error *error); -int session_send_upgrade_reply(Session *s, sd_bus_error *error); +int session_send_create_reply(Session *s, const sd_bus_error *error); +int session_send_upgrade_reply(Session *s, const sd_bus_error *error); int bus_session_method_activate(sd_bus_message *message, void *userdata, sd_bus_error *error); int bus_session_method_lock(sd_bus_message *message, void *userdata, sd_bus_error *error); From 394a6ab68cf6eafca20cc3391e90c8d67e88e9f9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 19 Nov 2024 23:23:21 +0100 Subject: [PATCH 4/4] logind: also potentially GC the session if we cannot send reply --- src/login/logind-dbus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index dc5c054d47..56531a22f6 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -1152,7 +1152,7 @@ static int create_session( * all is complete - or wait again. */ r = session_send_create_reply(session, /* error= */ NULL); if (r < 0) - return r; + goto fail; return 1;