refactor: simplify the daemon, fail unanswered greeter PAM prompts and close PAM on stop

daemon/daemon.cpp

@@ -54,8 +54,7 @@ Daemon::Daemon(Config config, QObject* parent)
     connect(&m_server, &GreeterServer::startSession, this, &Daemon::startSession);
     connect(&m_server, &GreeterServer::cancelSession, this, &Daemon::cancelSession);
     connect(&m_server, &GreeterServer::disconnected, this, [this] {
-        if (m_login && !m_login->hasStarted())
-            release(m_login);
+        release(m_login);
     });
 }
 
@@ -77,10 +76,9 @@ bool Daemon::start()
         qCritical() << "No login screen user" << m_config.greeterUser;
         return false;
     }
-    m_greeterUid = *greeter;
     QDir().mkpath(m_config.runtimeDirectory);
     const QString socket = m_config.runtimeDirectory + QStringLiteral("/greeter.sock");
-    if (!m_server.listen(socket, m_greeterUid)) {
+    if (!m_server.listen(socket, *greeter)) {
         qCritical() << "Cannot listen on" << socket;
         return false;
     }
@@ -112,8 +110,7 @@ void Daemon::startGreeter()
         m_greeter->start(m_config.greeterCommand,
                          greeterEnvironment(m_config.greeterVt, m_server.path()));
     });
-    connect(m_greeter.get(), &HelperProcess::started, this, [this](const QString& id) {
-        m_greeterSessionId = id;
+    connect(m_greeter.get(), &HelperProcess::started, this, [this] {
         if (std::exchange(m_bootSplash, false))
             QProcess::startDetached(QStringLiteral("plymouth"),
                                     {QStringLiteral("quit"), QStringLiteral("--retain-splash")});
@@ -139,7 +136,7 @@ void Daemon::showGreeter()
     whenControlled(greeter, m_config.greeterVt, [this, greeter](bool controlled) {
         if (m_greeter.get() != greeter)
             return;
-        if (controlled && activateSession(m_greeterSessionId)) {
+        if (controlled && activateSession(greeter->sessionId())) {
             replySwitchRequests(true);
             return;
         }
@@ -156,10 +153,22 @@ void Daemon::stopGreeter()
         m_greeter->terminate();
 }
 
+void Daemon::returnToGreeter()
+{
+    // A login screen that already handed a login over is replaced, not shown.
+    if (m_greeter && m_greeterSpent) {
+        m_restartGreeter = true;
+        stopGreeter();
+    } else if (m_greeter) {
+        showGreeter();
+    } else {
+        startGreeter();
+    }
+}
+
 void Daemon::onGreeterFinished()
 {
     release(m_greeter);
-    m_greeterSessionId.clear();
     restoreTextMode(m_config.greeterVt);
     if (m_stopping)
         return;
@@ -202,21 +211,12 @@ void Daemon::SwitchToGreeter(const QDBusMessage& message)
     }
     message.setDelayedReply(true);
     m_switchRequests.append(message);
-    // A login screen that already handed a login over is replaced, not shown.
-    if (m_greeter && m_greeterSpent) {
-        m_restartGreeter = true;
-        stopGreeter();
-    } else if (m_greeter) {
-        showGreeter();
-    } else {
-        startGreeter();
-    }
+    returnToGreeter();
 }
 
 void Daemon::createSession(const QString& username)
 {
-    if (m_login)
-        release(m_login);
+    release(m_login);
     m_login = std::make_unique<HelperProcess>();
     auto* login = m_login.get();
     connect(login, &HelperProcess::question, this, [this](const QString& style, const QString& text) {
@@ -285,8 +285,7 @@ void Daemon::startSession(const QStringList& command, const QStringList& environ
 
 void Daemon::cancelSession()
 {
-    if (m_login && !m_login->hasStarted())
-        release(m_login);
+    release(m_login);
     m_server.respond(GreeterServer::success());
 }
 
@@ -331,17 +330,9 @@ void Daemon::onSessionFinished()
         launchSession(std::move(*pending));
         return;
     }
-    // A session that failed before it was shown leaves a spent login screen.
-    if (m_greeter && m_greeterSpent) {
-        m_restartGreeter = true;
-        stopGreeter();
-        return;
-    }
-    // A crash, or another desktop's own logout: the login screen comes back.
-    if (m_greeter)
-        showGreeter();
-    else
-        startGreeter();
+    // A crash, another desktop's own logout, or a session that failed before
+    // it was shown: the login screen comes back.
+    returnToGreeter();
 }
 
 void Daemon::whenControlled(HelperProcess* helper, int vt, std::function<void(bool)> then)

daemon/daemon.h

@@ -7,7 +7,6 @@
 #include <functional>
 #include <memory>
 #include <optional>
-#include <sys/types.h>
 
 #include "config.h"
 #include "greeterserver.h"
@@ -40,6 +39,7 @@ private:
     void startGreeter();
     void showGreeter();
     void stopGreeter();
+    void returnToGreeter();
     void onGreeterFinished();
     void replySwitchRequests(bool switched);
 
@@ -60,11 +60,9 @@ private:
     Config m_config;
     State m_state;
     GreeterServer m_server;
-    uid_t m_greeterUid = 0;
     std::unique_ptr<HelperProcess> m_greeter;
     std::unique_ptr<HelperProcess> m_login;
     std::unique_ptr<HelperProcess> m_session;
-    QString m_greeterSessionId;
     std::optional<PendingStart> m_pendingStart;
     QList<QDBusMessage> m_switchRequests;
     QTimer m_greeterRestart;

daemon/helperprocess.cpp

@@ -57,8 +57,11 @@ HelperProcess::~HelperProcess()
 {
     disconnect(&m_process, nullptr, this, nullptr);
     if (m_process.state() != QProcess::NotRunning) {
+        // EOF ends a helper waiting for a message even when SIGTERM lands just before its read().
+        m_process.closeWriteChannel();
         terminate();
-        if (!m_process.waitForFinished(3000))
+        // A helper that may own a session gets its whole bounded stop, which ends by closing PAM.
+        if (!m_process.waitForFinished(isAuthenticated() ? m_stopDeadline.interval() : 3000))
             m_process.kill();
         m_process.waitForFinished(1000);
     }

daemon/sessions.cpp

@@ -82,7 +82,6 @@ std::optional<SessionEntry> readSessionFile(const QString& path)
     SessionEntry entry;
     entry.id = QFileInfo(path).completeBaseName();
     entry.name = unescape(keys.value(QStringLiteral("Name"), entry.id));
-    entry.comment = unescape(keys.value(QStringLiteral("Comment")));
     // Field codes have no meaning for a session and are dropped.
     for (const QString& argument : QProcess::splitCommand(unescape(keys.value(QStringLiteral("Exec"))))) {
         if (!(argument.size() == 2 && argument.startsWith(QLatin1Char('%'))))

daemon/sessions.h

@@ -12,7 +12,6 @@ struct SessionEntry {
     // The file name without ".desktop", e.g. "cutefish-wayland".
     QString id;
     QString name;
-    QString comment;
     QStringList exec;
     QStringList desktopNames;
 };

daemon/state.cpp

@@ -11,18 +11,6 @@ State::State(QString path)
 {
 }
 
-QString State::lastUser() const
-{
-    return QSettings(m_path, QSettings::IniFormat).value(QStringLiteral("Last/User")).toString();
-}
-
-QString State::lastSession(const QString& user) const
-{
-    QSettings settings(m_path, QSettings::IniFormat);
-    settings.beginGroup(QStringLiteral("Sessions"));
-    return settings.value(user).toString();
-}
-
 void State::remember(const QString& user, const QString& session)
 {
     QDir().mkpath(QFileInfo(m_path).absolutePath());

daemon/state.h

@@ -11,8 +11,6 @@ class State
 public:
     explicit State(QString path);
 
-    QString lastUser() const;
-    QString lastSession(const QString& user) const;
     void remember(const QString& user, const QString& session);
 
 private:

helper/main.cpp

@@ -105,6 +105,9 @@ bool groupHasOthers(pid_t leader)
 
 std::optional<QJsonObject> receive()
 {
+    // A SIGTERM taken outside read() interrupted nothing; the message is not coming.
+    if (g_stopRequested)
+        return std::nullopt;
     std::string line;
     char c = 0;
     ssize_t count = 0;
@@ -180,7 +183,7 @@ int converse(int count, const pam_message** messages, pam_response** result, voi
             continue;
         }
         QString style;
-        switch (messages[i]->msg_style) {
+        switch (kind) {
         case PAM_PROMPT_ECHO_OFF: style = QStringLiteral("secret"); break;
         case PAM_PROMPT_ECHO_ON: style = QStringLiteral("visible"); break;
         case PAM_ERROR_MSG: style = QStringLiteral("error"); break;
@@ -197,8 +200,7 @@ int converse(int count, const pam_message** messages, pam_response** result, voi
             freeResponses(responses, count);
             return PAM_CONV_ERR;
         }
-        const int prompt = messages[i]->msg_style;
-        if (prompt == PAM_PROMPT_ECHO_OFF || prompt == PAM_PROMPT_ECHO_ON) {
+        if (kind == PAM_PROMPT_ECHO_OFF || kind == PAM_PROMPT_ECHO_ON) {
             QString text = answer->value(QStringLiteral("response")).toString();
             QByteArray bytes = text.toUtf8();
             responses[i].resp = strdup(bytes.constData());
@@ -275,6 +277,8 @@ int main()
         fail(false, QStringLiteral("Incomplete login request"));
         return 1;
     }
+    // The login screen's own login has nobody to answer its PAM messages either.
+    g_conversing = authenticate;
 
     const pam_conv conversation {converse, nullptr};
     pam_handle_t* pam = nullptr;

tests/dm_test.cpp

@@ -2,6 +2,7 @@
 #include <QFile>
 #include <QJsonArray>
 #include <QLocalSocket>
+#include <QSettings>
 #include <QSignalSpy>
 #include <QTemporaryDir>
 #include <QTest>
@@ -217,14 +218,17 @@ private slots:
     void stateRemembersTheLastLogin()
     {
         QTemporaryDir directory;
-        dm::State state(directory.filePath(QStringLiteral("state/state.conf")));
-        QVERIFY(state.lastUser().isEmpty());
+        const QString path = directory.filePath(QStringLiteral("state/state.conf"));
+        dm::State state(path);
+        QVERIFY(!QFile::exists(path));
         state.remember(QStringLiteral("alice"), QStringLiteral("gnome"));
         state.remember(QStringLiteral("bob"), QStringLiteral("cutefish-wayland"));
-        dm::State reread(directory.filePath(QStringLiteral("state/state.conf")));
-        QCOMPARE(reread.lastUser(), QStringLiteral("bob"));
-        QCOMPARE(reread.lastSession(QStringLiteral("alice")), QStringLiteral("gnome"));
-        QCOMPARE(reread.lastSession(QStringLiteral("bob")), QStringLiteral("cutefish-wayland"));
+        // The keys the login screen reads.
+        const QSettings reread(path, QSettings::IniFormat);
+        QCOMPARE(reread.value(QStringLiteral("Last/User")).toString(), QStringLiteral("bob"));
+        QCOMPARE(reread.value(QStringLiteral("Sessions/alice")).toString(), QStringLiteral("gnome"));
+        QCOMPARE(reread.value(QStringLiteral("Sessions/bob")).toString(),
+                 QStringLiteral("cutefish-wayland"));
     }
 
     void configFallsBackToDefaults()