diff --git a/changelog/next.md b/changelog/next.md index 589d7c327..bbba7dd0b 100644 --- a/changelog/next.md +++ b/changelog/next.md @@ -5,6 +5,7 @@ ## Bug Fixes +- Fixed PAM helpers surviving a killed or crashed shell and PAM module crashes relaunching the shell. - Fixed main process crashes on pam subprocess misbehavior. - Fixed crashes when attempting to create or modify session locks reentrantly. - Fixed networking state breaking after restarting NetworkManager. diff --git a/src/services/pam/conversation.cpp b/src/services/pam/conversation.cpp index 1fb4c0415..42151b1c2 100644 --- a/src/services/pam/conversation.cpp +++ b/src/services/pam/conversation.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #ifdef __FreeBSD__ #include #endif @@ -36,10 +37,14 @@ QString PamResult::toString(PamResult::Enum value) { } } -PamConversation::~PamConversation() { this->abort(); } +PamConversation::~PamConversation() { + this->abort(); + if (this->lifelineFd != -1) close(this->lifelineFd); +} void PamConversation::start(const QString& configDir, const QString& config, const QString& user) { - this->childPid = PamConversation::createSubprocess(&this->pipes, configDir, config, user); + this->childPid = + PamConversation::createSubprocess(&this->pipes, &this->lifelineFd, configDir, config, user); if (this->childPid == 0) { qCCritical(logPam) << "Failed to create pam subprocess."; emit this->error(PamError::InternalError); diff --git a/src/services/pam/conversation.hpp b/src/services/pam/conversation.hpp index 779e6f472..5548f4ee5 100644 --- a/src/services/pam/conversation.hpp +++ b/src/services/pam/conversation.hpp @@ -87,6 +87,7 @@ private slots: private: static pid_t createSubprocess( PamIpcPipes* pipes, + int* lifelineFd, const QString& configDir, const QString& config, const QString& user @@ -95,6 +96,8 @@ private slots: void internalError(); pid_t childPid = 0; + // The subprocess is killed once every copy of this fd is closed, or if it is written to. + int lifelineFd = -1; PamIpcPipes pipes; QSocketNotifier notifier {QSocketNotifier::Read}; }; diff --git a/src/services/pam/subprocess.cpp b/src/services/pam/subprocess.cpp index 0ed579391..087da7294 100644 --- a/src/services/pam/subprocess.cpp +++ b/src/services/pam/subprocess.cpp @@ -1,8 +1,10 @@ #include "subprocess.hpp" #include +#include #include #include +#include #include #include #include @@ -18,16 +20,72 @@ #include "conversation.hpp" #include "ipc.hpp" +namespace { + +// Handlers installed by the shell, such as its crash handler, must not run in the subprocess. +// Reset them as exec would, leaving ignored signals ignored. +void resetSignalHandlers() { + // NOLINTBEGIN (misc-include-cleaner) + for (auto sig = 1; sig < NSIG; sig++) { + struct sigaction action {}; + if (sigaction(sig, nullptr, &action) == -1) continue; + if (action.sa_handler != SIG_DFL && action.sa_handler != SIG_IGN) signal(sig, SIG_DFL); + } + // NOLINTEND (misc-include-cleaner) +} + +#ifdef __FreeBSD__ +void onLifelineClosed(int /*sig*/) { _exit(1); } +#endif + +// Pipes raise SIGIO for an O_ASYNC reader when their last writer closes (Linux pipe_release, +// FreeBSD pipeclose), so this kills the subprocess once every copy of the lifeline's write end +// is gone. Unlike a parent death signal, that also happens when the shell relaunches in place +// after a crash, as exec closes the shell's copy. Writing to the lifeline also triggers it. +void armLifeline(int fd) { + // NOLINTBEGIN (misc-include-cleaner) + fcntl(fd, F_SETOWN, getpid()); + +#ifdef __FreeBSD__ + // There is no F_SETSIG, and SIGIO is ignored by default. + struct sigaction action {}; + action.sa_handler = &onLifelineClosed; + sigemptyset(&action.sa_mask); + sigaction(SIGIO, &action, nullptr); + + sigset_t mask {}; + sigemptyset(&mask); + sigaddset(&mask, SIGIO); + sigprocmask(SIG_UNBLOCK, &mask, nullptr); +#else + // Unlike SIGIO, a pam module cannot catch, block or ignore this. + fcntl(fd, F_SETSIG, SIGKILL); +#endif + + auto flags = fcntl(fd, F_GETFL); + if (flags != -1) fcntl(fd, F_SETFL, flags | O_ASYNC); + // NOLINTEND (misc-include-cleaner) +} + +} // namespace + pid_t PamConversation::createSubprocess( PamIpcPipes* pipes, + int* lifelineFd, const QString& configDir, const QString& config, const QString& user ) { auto toSubprocess = std::array(); auto fromSubprocess = std::array(); + auto lifeline = std::array(); - if (pipe(toSubprocess.data()) == -1 || pipe(fromSubprocess.data()) == -1) { + // The lifeline must be CLOEXEC so exec drops the shell's end, including the crash handler's + // in-place relaunch. Forks that never exec (later subprocesses, the crash handler's coredump + // child) keep a copy until they exit. + if (pipe(toSubprocess.data()) == -1 || pipe(fromSubprocess.data()) == -1 + || pipe2(lifeline.data(), O_CLOEXEC) == -1) + { qCDebug(logPam) << "Failed to create pipes for subprocess."; return 0; } @@ -42,6 +100,13 @@ pid_t PamConversation::createSubprocess( if (pid < 0) { qCDebug(logPam) << "Failed to fork for subprocess."; } else if (pid == 0) { + resetSignalHandlers(); + + // Armed before our copy of the write end is closed, so if the shell is already gone, + // closing it triggers the lifeline. + armLifeline(lifeline[0]); + close(lifeline[1]); // close w + close(toSubprocess[1]); // close w close(fromSubprocess[0]); // close r @@ -60,9 +125,11 @@ pid_t PamConversation::createSubprocess( } else { close(toSubprocess[0]); // close r close(fromSubprocess[1]); // close w + close(lifeline[0]); // close r pipes->fdIn = fromSubprocess[0]; pipes->fdOut = toSubprocess[1]; + *lifelineFd = lifeline[1]; free(configDirF); // NOLINT free(configF); // NOLINT