xss-lock shouldn't exit with SIGABRT on X connection termination

[Неточка Незванова]

As it is not a programming error, yet it produces a core dump every time X exits.

[Joan Bruguera]

I am also getting a log message in journalctl sometimes, when shutting down Xorg:

Jan 04 18:45:44 SOL audit[753]: ANOM_ABEND auid=1000 uid=1000 gid=1000 ses=2 pid=753 comm="xss-lock" exe="/usr/bin/xss-lock" sig=5 res=1
Jan 04 18:45:44 SOL kernel: traps: xss-lock[753] trap int3 ip:7fc65197b166 sp:7ffc009da940 error:0 in libglib-2.0.so.0.6200.4[7fc651933000+81000]

‌

The signal is a SIGTRAP instead of SIGABRT, however. To reproduce the error in gdb without killing Xorg, find the FD of the Xorg socket (lsof +E -aUc Xorg), break in xss-lock inside GDB and and force close it (p (int)close($FD)).

‌

(gdb) p (int)close(3)
$1 = 0
(gdb) continue
Continuing.

** (xss-lock:51314): CRITICAL **: 19:04:44.738: X connection lost; exiting.

Thread 1 "xss-lock" received signal SIGTRAP, Trace/breakpoint trap.
0x00007ffff7cc9166 in ?? () from /usr/lib/libglib-2.0.so.0
(gdb) bt
#0 0x00007ffff7cc9166 in () at /usr/lib/libglib-2.0.so.0
#1 0x00007ffff7cc93b8 in g_logv () at /usr/lib/libglib-2.0.so.0
#2 0x00007ffff7cc95a0 in g_log () at /usr/lib/libglib-2.0.so.0
#3 0x000055555555815e in screensaver_event_cb (connection=0x55555556b4d0, event=0x0, xcb_screensaver_notify=0x555555581d7a) at /usr/src/debug/raymonad-xss-lock-1e158fb20108/src/xss-lock.c:162
#4 0x000055555555850f in xcb_event_dispatch (source=0x55555557faa0, callback=0x555555557fc0 <screensaver_event_cb>, user_data=0x555555581d7a) at /usr/src/debug/raymonad-xss-lock-1e158fb20108/src/xcb_utils.c:90
#5 0x00007ffff7ccf26f in g_main_context_dispatch () at /usr/lib/libglib-2.0.so.0
#6 0x00007ffff7cd11b1 in () at /usr/lib/libglib-2.0.so.0
#7 0x00007ffff7cd20c3 in g_main_loop_run () at /usr/lib/libglib-2.0.so.0
#8 0x00005555555573e3 in main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/raymonad-xss-lock-1e158fb20108/src/xss-lock.c:528

‌

The problem is simply that xss-lock calls g_critical if the Xorg connection breaks, but a more graceful way to shutdown should be used instead.

g_critical()

“Critical warnings are intended to be used in the event of an error that originated in the current process (a programmer error). Logging of a critical error is by definition an indication of a bug somewhere in the current program (or its libraries).”

‌

[Joan Bruguera]

Tentative patch:

‌

diff --git a/src/xss-lock.c b/src/xss-lock.c
index cbcd38a..ad9b206 100644
--- a/src/xss-lock.c
+++ b/src/xss-lock.c
@@ -53,9 +53,10 @@ static void logind_session_set_idle_hint(gboolean idle);
 static gboolean parse_options(int argc, char *argv[], GError **error);
 static gboolean parse_notifier_cmd(const gchar *option_name, const gchar *value, gpointer data, GError **error);
 static gboolean reset_screensaver(xcb_connection_t *connection);
-static gboolean exit_service(GMainLoop *loop);
+static gboolean exit_service();
 static void log_handler(const gchar *log_domain, GLogLevelFlags log_level, const gchar *message, gpointer user_data);

+static GMainLoop *loop;
 static Child notifier = {"notifier", NULL, 0, FALSE, NULL};
 static Child locker = {"locker", NULL, 0, FALSE, &notifier};
 static gboolean opt_quiet = FALSE;
@@ -158,8 +159,11 @@ screensaver_event_cb(xcb_connection_t *connection, xcb_generic_event_t *event,
 {
     uint8_t event_type;

-    if (!event)
-        g_critical("X connection lost; exiting.");
+    if (!event) {
+        g_info("X connection lost; exiting.");
+        exit_service();
+        return TRUE;
+    }

     event_type = XCB_EVENT_RESPONSE_TYPE(event);
     if (event_type == 0) {
@@ -462,7 +466,7 @@ reset_screensaver(xcb_connection_t *connection)
 }

 static gboolean
-exit_service(GMainLoop *loop)
+exit_service()
 {
     kill_child(&notifier);
     kill_child(&locker);
@@ -482,7 +486,6 @@ log_handler(const gchar *log_domain, GLogLevelFlags log_level,
 int
 main(int argc, char *argv[])
 {
-    GMainLoop *loop;
     GError *error = NULL;
     xcb_connection_t *connection = NULL;
     int default_screen_number;
@@ -517,9 +520,9 @@ main(int argc, char *argv[])
                              NULL, logind_manager_proxy_new_cb, NULL);

     loop = g_main_loop_new(NULL, FALSE);
-    g_unix_signal_add(SIGTERM, (GSourceFunc)exit_service, loop);
-    g_unix_signal_add(SIGINT,  (GSourceFunc)exit_service, loop);
-    g_unix_signal_add(SIGHUP,  (GSourceFunc)exit_service, loop);
+    g_unix_signal_add(SIGTERM, (GSourceFunc)exit_service, NULL);
+    g_unix_signal_add(SIGINT,  (GSourceFunc)exit_service, NULL);
+    g_unix_signal_add(SIGHUP,  (GSourceFunc)exit_service, NULL);

     default_screen = xcb_aux_get_screen(connection, default_screen_number);
     if (!register_screensaver(connection, default_screen, &atom, &error))
-- 
2.24.1

‌

[Joan Bruguera]

A different patch for this issue has been applied to the package in Debian and also in Arch:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=955209

‌