fix: signal a lock screen only once it handles the signal

CMakeLists.txt

@@ -151,6 +151,7 @@ if(FU_BUILD_AGENT)
         src/agent/bubblecontroller.cpp
         src/agent/lockcontroller.cpp
         src/agent/lockwatcher.cpp
+        src/agent/lockers.cpp
         src/agent/inputwatcher.cpp
         src/agent/wayland.cpp
         src/agent/enrollcontroller.cpp
@@ -219,4 +220,11 @@ if(FU_BUILD_TESTS)
 
     add_executable(test_images tests/test_images.cpp)
     target_link_libraries(test_images PRIVATE fu_core)
+
+    if(FU_BUILD_AGENT)
+        add_executable(test_lockers tests/test_lockers.cpp src/agent/lockers.cpp)
+        target_include_directories(test_lockers PRIVATE src/agent)
+        target_link_libraries(test_lockers PRIVATE Qt6::Core)
+        add_test(NAME lockers COMMAND test_lockers)
+    endif()
 endif()

src/agent/lockers.cpp

@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+#include "lockers.h"
+
+#include <QFile>
+#include <QString>
+
+#include <csignal>
+#include <dirent.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+namespace
+{
+bool isLocker(const QByteArray &name)
+{
+    return name == "hyprlock" || name == "swaylock";
+}
+
+QByteArray readFile(const QString &path)
+{
+    QFile f(path);
+    return f.open(QIODevice::ReadOnly) ? f.readAll() : QByteArray();
+}
+
+pid_t parentOf(pid_t pid)
+{
+    // The fields after the name in brackets, which can hold anything.
+    const QByteArray stat = readFile(QStringLiteral("/proc/%1/stat").arg(pid));
+    const qsizetype close = stat.lastIndexOf(')');
+    return close < 0 ? 0 : pid_t(stat.mid(close + 2).split(' ').value(1).toInt());
+}
+
+// The lock screens of this user on this display. Another session of the same
+// user has a display of its own, and its lock screen is left alone. A child
+// of a lock screen (swaylock checks the password in one) is left out: killed
+// by the signal before its parent unlocks, it would take the parent down
+// with it, and the screen would stay locked.
+QList<pid_t> findAll()
+{
+    QList<pid_t> found;
+    DIR *proc = ::opendir("/proc");
+    if (!proc) {
+        return found;
+    }
+    QByteArray display = qgetenv("WAYLAND_DISPLAY");
+    if (display.isEmpty()) {
+        display = "wayland-0";
+    }
+    const uid_t me = ::getuid();
+    while (dirent *entry = ::readdir(proc)) {
+        const pid_t pid = pid_t(atoi(entry->d_name));
+        struct stat st;
+        if (pid <= 0 || ::fstatat(::dirfd(proc), entry->d_name, &st, 0) != 0 || st.st_uid != me) {
+            continue;
+        }
+        const QString dir = QStringLiteral("/proc/%1/").arg(pid);
+        if (!isLocker(readFile(dir + QStringLiteral("comm")).trimmed())) {
+            continue;
+        }
+        bool sameDisplay = true;
+        for (const QByteArray &var : readFile(dir + QStringLiteral("environ")).split('\0')) {
+            if (var.startsWith("WAYLAND_DISPLAY=")) {
+                sameDisplay = var.mid(16) == display;
+                break;
+            }
+        }
+        if (sameDisplay) {
+            found.append(pid);
+        }
+    }
+    ::closedir(proc);
+    QList<pid_t> top;
+    for (const pid_t pid : std::as_const(found)) {
+        if (!found.contains(parentOf(pid))) {
+            top.append(pid);
+        }
+    }
+    return top;
+}
+
+// Whether pid catches sig, from the mask in /proc/<pid>/status.
+bool catches(pid_t pid, int sig)
+{
+    for (const QByteArray &line : readFile(QStringLiteral("/proc/%1/status").arg(pid)).split('\n')) {
+        if (line.startsWith("SigCgt:")) {
+            bool ok = false;
+            const qulonglong mask = line.mid(7).trimmed().toULongLong(&ok, 16);
+            return ok && (mask >> (sig - 1)) & 1;
+        }
+    }
+    return false;
+}
+} // namespace
+
+QList<pid_t> Lockers::find(const QByteArray &name)
+{
+    QList<pid_t> found;
+    for (const pid_t pid : findAll()) {
+        if (name.isEmpty() || readFile(QStringLiteral("/proc/%1/comm").arg(pid)).trimmed() == name) {
+            found.append(pid);
+        }
+    }
+    return found;
+}
+
+int Lockers::signal(int sig, const QByteArray &name)
+{
+    int waiting = 0;
+    for (const pid_t pid : find(name)) {
+        if (catches(pid, sig)) {
+            ::kill(pid, sig);
+        } else {
+            ++waiting;
+        }
+    }
+    return waiting;
+}

src/agent/lockers.h

@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+//
+// The lock screens that are programs of their own and take signals from
+// outside: hyprlock and swaylock open on SIGUSR1.
+
+#pragma once
+
+#include <QByteArray>
+#include <QList>
+
+#include <sys/types.h>
+
+namespace Lockers
+{
+// The ones of this user on this display, or only those called name.
+QList<pid_t> find(const QByteArray &name = {});
+// Sends sig to each that is ready for it, and says how many were not. A
+// lock screen only handles its signals once the screen is locked: before
+// that, the signal kills it, and the compositor keeps the screen locked with
+// nobody to open it.
+int signal(int sig, const QByteArray &name = {});
+}

src/agent/lockwatcher.cpp

@@ -2,6 +2,7 @@
 
 #include "lockwatcher.h"
 
+#include "lockers.h"
 #include "wayland.h"
 
 #include <QDBusConnection>
@@ -10,12 +11,9 @@
 #include <QDBusPendingCallWatcher>
 #include <QDBusPendingReply>
 #include <QDBusVariant>
-#include <QFile>
 #include <QProcess>
 
-#include <dirent.h>
-#include <signal.h>
-#include <sys/stat.h>
+#include <csignal>
 #include <unistd.h>
 
 namespace
@@ -26,73 +24,6 @@ const QString Properties = QStringLiteral("org.freedesktop.DBus.Properties");
 
 constexpr int PollMs = 1000;
 
-// The lock screens that unlock on SIGUSR1.
-bool isLocker(const QByteArray &name)
-{
-    return name == "hyprlock" || name == "swaylock";
-}
-
-QByteArray readFile(const QString &path)
-{
-    QFile f(path);
-    return f.open(QIODevice::ReadOnly) ? f.readAll() : QByteArray();
-}
-
-pid_t parentOf(pid_t pid)
-{
-    // The fields after the name in brackets, which can hold anything.
-    const QByteArray stat = readFile(QStringLiteral("/proc/%1/stat").arg(pid));
-    const qsizetype close = stat.lastIndexOf(')');
-    return close < 0 ? 0 : pid_t(stat.mid(close + 2).split(' ').value(1).toInt());
-}
-
-// The lock screens of this user on this display. Another session of the same
-// user has a display of its own, and its lock screen is left alone. A child
-// of a lock screen (swaylock checks the password in one) is left out: killed
-// by the signal before its parent unlocks, it would take the parent down
-// with it, and the screen would stay locked.
-QList<pid_t> findLockers()
-{
-    QList<pid_t> found;
-    DIR *proc = ::opendir("/proc");
-    if (!proc) {
-        return found;
-    }
-    QByteArray display = qgetenv("WAYLAND_DISPLAY");
-    if (display.isEmpty()) {
-        display = "wayland-0";
-    }
-    const uid_t me = ::getuid();
-    while (dirent *entry = ::readdir(proc)) {
-        const pid_t pid = pid_t(atoi(entry->d_name));
-        struct stat st;
-        if (pid <= 0 || ::fstatat(::dirfd(proc), entry->d_name, &st, 0) != 0 || st.st_uid != me) {
-            continue;
-        }
-        const QString dir = QStringLiteral("/proc/%1/").arg(pid);
-        if (!isLocker(readFile(dir + QStringLiteral("comm")).trimmed())) {
-            continue;
-        }
-        bool sameDisplay = true;
-        for (const QByteArray &var : readFile(dir + QStringLiteral("environ")).split('\0')) {
-            if (var.startsWith("WAYLAND_DISPLAY=")) {
-                sameDisplay = var.mid(16) == display;
-                break;
-            }
-        }
-        if (sameDisplay) {
-            found.append(pid);
-        }
-    }
-    ::closedir(proc);
-    QList<pid_t> top;
-    for (const pid_t pid : std::as_const(found)) {
-        if (!found.contains(parentOf(pid))) {
-            top.append(pid);
-        }
-    }
-    return top;
-}
 } // namespace
 
 LockWatcher::LockWatcher(QObject *parent)
@@ -226,7 +157,7 @@ void LockWatcher::onSessionProperties(const QString &interface, const QVariantMa
 
 void LockWatcher::pollLockers()
 {
-    m_lockerRunning = !findLockers().isEmpty();
+    m_lockerRunning = !Lockers::find().isEmpty();
     update();
 }
 
@@ -243,9 +174,7 @@ void LockWatcher::unlock()
 {
     // Looked up again rather than taken from the last poll: a second is
     // long enough for a pid to belong to something else.
-    for (const pid_t pid : findLockers()) {
-        ::kill(pid, SIGUSR1);
-    }
+    Lockers::signal(SIGUSR1);
 
     const QDBusMessage call = QDBusMessage::createMethodCall(Login1,
                                                              m_session.isEmpty() ? QStringLiteral("/org/freedesktop/login1/session/auto") : m_session,

tests/test_lockers.cpp

@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+//
+// A lock screen gets a signal only once it handles it. Before that, the
+// signal would kill it and leave the screen locked for good. The lock
+// screen here is a child of the test that calls itself hyprlock, on a
+// display of its own, so no real one is ever found. The display is set by
+// starting the test again: /proc shows the environment a program started
+// with, not what it set later.
+
+#include "lockers.h"
+
+#include <QtGlobal>
+
+#include <csignal>
+#include <cstdlib>
+#include <cstdio>
+#include <poll.h>
+#include <sys/prctl.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+namespace
+{
+int failures = 0;
+
+void check(bool ok, const char *what)
+{
+    std::printf("%s  %s\n", ok ? "ok  " : "FAIL", what);
+    if (!ok) {
+        ++failures;
+    }
+}
+
+int toParent = -1;
+
+void answer(int)
+{
+    [[maybe_unused]] const ssize_t n = write(toParent, "s", 1);
+}
+
+// A byte from the child, or 0 after a second without one.
+char next(int from)
+{
+    pollfd p{from, POLLIN, 0};
+    char c = 0;
+    if (poll(&p, 1, 1000) == 1 && read(from, &c, 1) == 1) {
+        return c;
+    }
+    return 0;
+}
+
+// A hyprlock that handles SIGUSR2 or not. Says "r" when it is set up, and
+// "s" for every SIGUSR2.
+pid_t fakeHyprlock(bool handles, int *fromChild)
+{
+    int fds[2];
+    if (pipe(fds) != 0) {
+        return -1;
+    }
+    const pid_t pid = fork();
+    if (pid == 0) {
+        close(fds[0]);
+        toParent = fds[1];
+        prctl(PR_SET_NAME, "hyprlock");
+        if (handles) {
+            std::signal(SIGUSR2, answer);
+        }
+        [[maybe_unused]] const ssize_t n = write(toParent, "r", 1);
+        for (;;) {
+            pause();
+        }
+    }
+    close(fds[1]);
+    *fromChild = fds[0];
+    next(fds[0]);
+    return pid;
+}
+
+bool alive(pid_t pid)
+{
+    return waitpid(pid, nullptr, WNOHANG) == 0;
+}
+} // namespace
+
+int main(int, char **argv)
+{
+    const char *display = "fu-test-lockers";
+    if (qgetenv("WAYLAND_DISPLAY") != display) {
+        setenv("WAYLAND_DISPLAY", display, 1);
+        execv("/proc/self/exe", argv);
+        return 2;
+    }
+
+    int from = -1;
+    pid_t pid = fakeHyprlock(false, &from);
+    check(Lockers::find("hyprlock") == QList<pid_t>{pid}, "finds the lock screen on this display");
+    check(Lockers::find("swaylock").isEmpty(), "and only by that name");
+    check(Lockers::signal(SIGUSR2, "hyprlock") == 1, "one not ready yet");
+    usleep(200 * 1000);
+    check(alive(pid), "and it was left alone, alive");
+    kill(pid, SIGKILL);
+    waitpid(pid, nullptr, 0);
+    close(from);
+
+    pid = fakeHyprlock(true, &from);
+    check(Lockers::signal(SIGUSR2, "hyprlock") == 0, "a ready one gets it");
+    check(next(from) == 's', "and handles it");
+    check(alive(pid), "and lives on");
+    kill(pid, SIGKILL);
+    waitpid(pid, nullptr, 0);
+    close(from);
+
+    std::printf("\n%s\n", failures ? "SOME CHECKS FAILED" : "all checks passed");
+    return failures ? 1 : 0;
+}