Repository navigation
fix: second user cannot login & misbehaved logout - #71
Conversation
This will work as intended.
|
Hi @calsys456. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's GuideRefactors how user sessions are spawned and tracked so that PAM session opening, XDG_SESSION_ID, and utmp handling occur in a dedicated forked session-leader process, fixes PID tracking for multi-user logins, and simplifies logout/termination handling by relying on per-session cleanup instead of global signal handling and logind KillSession. Sequence diagram for the new session leader based user login flowsequenceDiagram
actor User
participant Greeter
participant Display
participant Auth
participant PAM
participant VirtualTerminal
participant SessionLeader
participant UserSession
participant Logind
User->>Greeter: Request login (user, secret)
Greeter->>Display: login(user, secret)
Display->>Auth: authenticate(secret)
Auth->>PAM: pam_start / pam_authenticate / pam_acct_mgmt
PAM-->>Auth: PAM_SUCCESS
Auth-->>Display: authenticated = true
Display->>Auth: openSession(command, type, env, cookie)
Auth->>Auth: pipe(pipefd)
Auth->>VirtualTerminal: jumpToVt(tty, false)
Auth->>SessionLeader: fork() as sessionLeaderPid
alt fork failed
Auth-->>Display: return -1
else session leader (child)
Auth->>SessionLeader: setup
SessionLeader->>Auth: getpwnam(user)
SessionLeader->>SessionLeader: fill env (HOME, SHELL, USER, LOGNAME)
SessionLeader->>PAM: openSessionInternal(env)
PAM-->>SessionLeader: updated env with XDG_SESSION_ID
SessionLeader->>SessionLeader: xdgSessionId = env[XDG_SESSION_ID]
SessionLeader->>Auth: write xdgSessionId to pipe
SessionLeader->>UserSession: create
SessionLeader->>UserSession: start(command, type, cookie)
UserSession-->>SessionLeader: waitForStarted()
SessionLeader->>Auth: write sessionPid to pipe
SessionLeader->>UserSession: waitForFinished(-1)
UserSession-->>SessionLeader: exitCode / exitStatus
SessionLeader->>SessionLeader: exit(exitCode)
else parent (Auth)
Auth->>Auth: read xdgSessionId from pipe
Auth->>Auth: read sessionPid from pipe
Auth->>Auth: utmpLogin(true)
Auth->>Logind: GetSessionByPID(sessionPid) (indirect via PAM env)
Auth->>Display: return xdgSessionId
Auth->>Auth: m_notifier = QSocketNotifier(pipefd[0])
Auth->>Display: Q_EMIT sessionFinished() when notifier fires
end
Display->>Display: track auth in auths list
Display->>Logind: activateSession(user, xdgSessionId)
Updated class diagram for Auth, Display, UserSession, and DaemonAppclassDiagram
class Auth {
+QString user
+int tty
+bool authenticated
+bool sessionOpened
+int sessionId
+int xdgSessionId
+pid_t sessionLeaderPid
+qint64 sessionPid
+Auth(QObject *parent, QString user)
+~Auth()
+bool authenticate(const QByteArray &secret)
+int openSession(const QStringList &command, int type, QProcessEnvironment env, const QByteArray &cookie)
+bool closeSession()
+void utmpLogin(bool writeWtmp)
+void utmpLogout()
+std::optional<QProcessEnvironment> openSessionInternal(const QProcessEnvironment &sessionEnv)
+Q_SIGNAL void sessionFinished()
}
class AuthPrivate {
+pam_handle_t *handle
+int ret
+QByteArray *secretPtr
}
class UserSession {
+UserSession(QObject *parent)
+void start(const QStringList &command, int type, const QByteArray &cookie)
+void stop()
+qint64 processId() const
+void setProcessEnvironment(const QProcessEnvironment &env)
+void childModifier()
}
class Display {
+QList~Auth*~ auths
+void login(QLocalSocket *socket, const QString &user, const QString &password)
+void logout(QLocalSocket *socket, int id)
+void unlock(QLocalSocket *socket, const QString &user, const QString &password)
}
class DaemonApp {
+DisplayManager *displayManager()
+SeatManager *seatManager()
+PowerManager *powerManager()
+TreelandConnector *treelandConnector() const
}
class QSocketNotifier {
+QSocketNotifier(int socket, Type type, QObject *parent)
+void setEnabled(bool enabled)
}
class DisplayManager {
+void AddSeat(const QString &seatId)
+void RemoveSeat(const QString &seatId)
+void RemoveSession(int sessionId)
}
Auth --> AuthPrivate : has
Auth --> QSocketNotifier : uses m_notifier
Auth --> UserSession : child creates session
Display --> Auth : manages multiple
Display --> DisplayManager : uses
DaemonApp --> DisplayManager : owns
DaemonApp --> SeatManager : owns
DaemonApp --> PowerManager : owns
DaemonApp --> TreelandConnector : owns
UserSession --> Auth : parent in childModifier
DisplayManager <.. Display : interface used
QSocketNotifier <.. Auth : monitors sessionLeader pipe
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- The new pipe/QSocketNotifier flow in
Auth::startSessionlooks flawed: the parent fully drains the pipe synchronously (reading both XDG_SESSION_ID and sessionPid) before creating theQSocketNotifier, so the notifier will never fire andsessionFinishedwill never be emitted; consider either not pre-reading, or using a separate mechanism (e.g.SIGCHLD/waitpid) to know when the session leader exits. - The pipe protocol assumes
read(pipefd[0], &xdgSessionId, sizeof(int))andread(pipefd[0], &sessionPid, sizeof(qint64))return the full object in one call; to make this robust, loop until the requested number of bytes is read or an error/EOF occurs and handle short reads explicitly. - The newly introduced
sessionLeaderPidis neverwaitpid'd, which may leak zombies; consider adding aSIGCHLDhandler or explicitwaitpid(sessionLeaderPid, ...)in the parent to reap the process when the session terminates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new pipe/QSocketNotifier flow in `Auth::startSession` looks flawed: the parent fully drains the pipe synchronously (reading both XDG_SESSION_ID and sessionPid) before creating the `QSocketNotifier`, so the notifier will never fire and `sessionFinished` will never be emitted; consider either not pre-reading, or using a separate mechanism (e.g. `SIGCHLD`/`waitpid`) to know when the session leader exits.
- The pipe protocol assumes `read(pipefd[0], &xdgSessionId, sizeof(int))` and `read(pipefd[0], &sessionPid, sizeof(qint64))` return the full object in one call; to make this robust, loop until the requested number of bytes is read or an error/EOF occurs and handle short reads explicitly.
- The newly introduced `sessionLeaderPid` is never `waitpid`'d, which may leak zombies; consider adding a `SIGCHLD` handler or explicit `waitpid(sessionLeaderPid, ...)` in the parent to reap the process when the session terminates.
## Individual Comments
### Comment 1
<location> `src/daemon/Auth.cpp:172-173` </location>
<code_context>
- if (m_session->state() != QProcess::NotRunning)
- m_session->stop();
if (sessionOpened) {
+ delete m_notifier;
+ qWarning() << "Killing" << sessionPid << "and" << sessionLeaderPid;
+ if (sessionPid > 0)
+ kill(sessionPid, SIGTERM);
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid manually deleting m_notifier since it has a QObject parent and may already be scheduled for deleteLater().
Because `m_notifier` has `this` as a QObject parent and is also destroyed via `deleteLater()` in the callback, adding `delete m_notifier;` introduces a double-delete risk. Either rely solely on QObject parent ownership / `deleteLater()` (and just null the pointer when queued), or remove the parent and handle deletion in exactly one place.
</issue_to_address>
### Comment 2
<location> `src/daemon/Auth.cpp:367-368` </location>
<code_context>
+ d->ret = pam_set_item(d->handle, PAM_TTY, qPrintable(vtPath));
CHECK_RET_OPEN
// Set PAM_XDISPLAY
- QString display = sessionEnv.value(QStringLiteral("DISPLAY"));
if (!display.isEmpty()) {
d->ret = pam_set_item(d->handle, PAM_XDISPLAY, qPrintable(display));
CHECK_RET_OPEN
</code_context>
<issue_to_address>
**issue (bug_risk):** display is used but no longer initialized locally in openSessionInternal.
This block still uses `display`, but the local `QString display = sessionEnv.value(QStringLiteral("DISPLAY"));` was removed. Unless `display` is a member on this class, this now fails to compile or refers to the wrong symbol. Please either reintroduce the local `display` definition before this block or use `sessionEnv.value(QStringLiteral("DISPLAY"))` directly in the condition and `pam_set_item` call.
</issue_to_address>
### Comment 3
<location> `src/daemon/Auth.cpp:315-319` </location>
<code_context>
+ utmpLogin(true);
+
+ // Monitor child process ends
+ m_notifier = new QSocketNotifier(pipefd[0], QSocketNotifier::Read, this);
+ connect(m_notifier, &QSocketNotifier::activated, this, [this] {
+ m_notifier->setEnabled(false);
+ m_notifier->deleteLater();
+ Q_EMIT sessionFinished();
+ });
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The read-end of the pipe is never closed on the happy path, which leaks a file descriptor.
In the parent success path, `pipefd[0]` is handed to `QSocketNotifier` but never closed, and `QSocketNotifier` doesn’t take ownership of the fd. This leaves the read end open for the lifetime of the process. Ensure it’s closed in a single, well-defined place (e.g. in the notifier callback after emitting `sessionFinished()` or in the `Auth` destructor).
</issue_to_address>
### Comment 4
<location> `src/daemon/Auth.cpp:271-280` </location>
<code_context>
+ qCritical() << "[SessionLeader] Invalid XDG_SESSION_ID from pam_open_session()";
+ exit(1);
+ }
+ write(pipefd[1], &xdgSessionId, sizeof(int));
+
+ // RUN!!!
+ UserSession session(this);
+ session.setProcessEnvironment(env);
+ session.start(command, type, cookie);
+ if (!session.waitForStarted()) {
+ qCritical() << "[SessionLeader] Failed to start session process. Exit now.";
+ exit(1);
+ }
+
+ // Send session PID to parent
+ sessionPid = session.processId();
+ write(pipefd[1], &sessionPid, sizeof(qint64));
+ qInfo() << "[SessionLeader] Session started with PID" << sessionPid;
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Check the result of pipe writes to avoid silently losing data on error or short write.
`write()`’s return value is ignored, so short writes or errors on a full/broken pipe will go unnoticed and the parent may read partial or garbage data for `xdgSessionId`/`sessionPid`. Please check that `write()` returns `sizeof(...)` and handle failures appropriately (e.g., log and abort).
Suggested implementation:
```cpp
if (write(pipefd[1], &xdgSessionId, sizeof(xdgSessionId)) != sizeof(xdgSessionId)) {
qCritical() << "[SessionLeader] Failed to write XDG_SESSION_ID to pipe. Exit now.";
exit(1);
}
```
```cpp
// Send session PID to parent
sessionPid = session.processId();
if (write(pipefd[1], &sessionPid, sizeof(sessionPid)) != sizeof(sessionPid)) {
qCritical() << "[SessionLeader] Failed to write session PID to pipe. Exit now.";
exit(1);
}
qInfo() << "[SessionLeader] Session started with PID" << sessionPid;
```
</issue_to_address>
### Comment 5
<location> `src/daemon/Auth.cpp:302-307` </location>
<code_context>
+ // Parent process
+ close(pipefd[1]);
+
+ if (read(pipefd[0], &xdgSessionId, sizeof(int)) < 0) {
+ qWarning() << "[Auth] Failed to read XDG_SESSION_ID from child process:" << strerror(errno);
+ close(pipefd[0]);
+ return -1;
+ }
+ if (read(pipefd[0], &sessionPid, sizeof(qint64)) < 0) {
+ qWarning() << "[Auth] Failed to read session PID from child process:" << strerror(errno);
+ close(pipefd[0]);
</code_context>
<issue_to_address>
**suggestion:** Differentiate between read error and short read/EOF when receiving data from the child.
`read` only checks for `< 0`, so short reads or EOF (`0`) aren’t handled. That can leave `xdgSessionId`/`sessionPid` partially initialized. Consider treating `<= 0` as failure, or better, verify `ret == sizeof(...)` for each read and treat mismatches as errors.
Suggested implementation:
```cpp
{
const ssize_t ret = read(pipefd[0], &xdgSessionId, sizeof(int));
if (ret < 0) {
qWarning() << "[Auth] Failed to read XDG_SESSION_ID from child process:" << strerror(errno);
close(pipefd[0]);
return -1;
} else if (ret == 0) {
qWarning() << "[Auth] Unexpected EOF while reading XDG_SESSION_ID from child process";
close(pipefd[0]);
return -1;
} else if (ret != static_cast<ssize_t>(sizeof(int))) {
qWarning() << "[Auth] Short read while reading XDG_SESSION_ID from child process. Expected"
<< sizeof(int) << "bytes, got" << ret;
close(pipefd[0]);
return -1;
}
}
```
```cpp
{
const ssize_t ret = read(pipefd[0], &sessionPid, sizeof(qint64));
if (ret < 0) {
qWarning() << "[Auth] Failed to read session PID from child process:" << strerror(errno);
close(pipefd[0]);
return -1;
} else if (ret == 0) {
qWarning() << "[Auth] Unexpected EOF while reading session PID from child process";
close(pipefd[0]);
return -1;
} else if (ret != static_cast<ssize_t>(sizeof(qint64))) {
qWarning() << "[Auth] Short read while reading session PID from child process. Expected"
<< sizeof(qint64) << "bytes, got" << ret;
close(pipefd[0]);
return -1;
}
}
```
1. Ensure `<unistd.h>` is included somewhere in the file so `read` and `ssize_t` are available (likely already true).
2. The snippet you pasted for the parent branch shows a duplicated `close(pipefd[1]);` and an unconditional `return -1;` – verify that this is just truncation and not current code; if it is current code, you may want to clean that up separately.
</issue_to_address>
### Comment 6
<location> `src/daemon/Auth.cpp:225` </location>
<code_context>
- m_session->start(command, type, cookie);
- if (!m_session->waitForStarted()) {
- qWarning() << "[Auth] Failed to start user process.";
+ int pipefd[2];
+ if (pipe(pipefd) == -1) {
+ qWarning() << "[Auth] pipe failed:" << strerror(errno);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the new environment-building, PAM env parsing, and fork/pipe logic into small helpers and relying on QObject parenting so `openSession` and the destructor stay focused and easier to read.
You can keep the new multi‑process behavior while reducing complexity by extracting a few focused helpers and using QObject parenting.
### 1. Extract passwd‑based env setup
This logic is currently inline in the child branch and mutates `env`. Making it a helper makes the child branch shorter and more readable:
```cpp
// In Auth.h (private)
QProcessEnvironment buildUserEnvironment(QProcessEnvironment baseEnv) const;
```
```cpp
// In Auth.cpp
QProcessEnvironment Auth::buildUserEnvironment(QProcessEnvironment env) const
{
if (struct passwd *pw = getpwnam(qPrintable(user))) {
const auto home = QString::fromLocal8Bit(pw->pw_dir);
const auto shell = QString::fromLocal8Bit(pw->pw_shell);
const auto name = QString::fromLocal8Bit(pw->pw_name);
env.insert(QStringLiteral("HOME"), home);
env.insert(QStringLiteral("PWD"), home);
env.insert(QStringLiteral("SHELL"), shell);
env.insert(QStringLiteral("USER"), name);
env.insert(QStringLiteral("LOGNAME"), name);
}
return env;
}
```
Usage in `openSession` (child branch):
```cpp
case 0: {
close(pipefd[0]);
env = buildUserEnvironment(env);
auto sessionEnv = openSessionInternal(env);
...
```
### 2. Separate PAM env extraction from `openSessionInternal`
`openSessionInternal` is doing both PAM session open and env parsing. The parsing can be extracted:
```cpp
// In Auth.h (private)
QProcessEnvironment buildEnvFromPam(pam_handle_t *handle);
```
```cpp
// In Auth.cpp
QProcessEnvironment Auth::buildEnvFromPam(pam_handle_t *handle)
{
QProcessEnvironment env;
char **envlist = pam_getenvlist(handle);
if (!envlist)
return env;
for (int i = 0; envlist[i] != nullptr; ++i) {
QString str = QString::fromLocal8Bit(envlist[i]);
const int equalPos = str.indexOf(QLatin1Char('='));
if (equalPos > 0)
env.insert(str.left(equalPos), str.mid(equalPos + 1));
free(envlist[i]);
}
free(envlist);
return env;
}
```
Then `openSessionInternal` becomes focused on PAM operations:
```cpp
std::optional<QProcessEnvironment>
Auth::openSessionInternal(const QProcessEnvironment &sessionEnv)
{
qInfo() << "[Auth] Opening session for user" << user;
d->ret = pam_setcred(d->handle, PAM_ESTABLISH_CRED);
CHECK_RET_OPEN
QString vtPath = VirtualTerminal::path(tty);
d->ret = pam_set_item(d->handle, PAM_TTY, qPrintable(vtPath));
CHECK_RET_OPEN
if (!display.isEmpty()) {
d->ret = pam_set_item(d->handle, PAM_XDISPLAY, qPrintable(display));
CHECK_RET_OPEN
}
for (const QString &s : sessionEnv.toStringList()) {
d->ret = pam_putenv(d->handle, qPrintable(s));
CHECK_RET_OPEN
}
d->ret = pam_open_session(d->handle, 0);
CHECK_RET_OPEN
qInfo() << "[Auth] Session opened.";
return buildEnvFromPam(d->handle);
}
```
### 3. Encapsulate fork/pipe into a small helper
You can keep `Auth::openSession` high‑level if you move the low‑level fork/pipe details into a helper that returns a small result struct:
```cpp
// In Auth.h (private)
struct SessionStartResult {
int xdgSessionId = -1;
qint64 sessionPid = -1;
int readFd = -1;
bool ok() const { return xdgSessionId > 0 && sessionPid > 0 && readFd >= 0; }
};
SessionStartResult startSessionLeader(const QString &command,
QProcessEnvironment env,
const QByteArray &cookie);
```
```cpp
// In Auth.cpp
Auth::SessionStartResult
Auth::startSessionLeader(const QString &command,
QProcessEnvironment env,
const QByteArray &cookie)
{
SessionStartResult result;
int pipefd[2];
if (pipe(pipefd) == -1) {
qWarning() << "[Auth] pipe failed:" << strerror(errno);
return result;
}
VirtualTerminal::jumpToVt(tty, false);
sessionLeaderPid = fork();
switch (sessionLeaderPid) {
case -1:
qWarning() << "[Auth] fork failed:" << strerror(errno);
close(pipefd[0]);
close(pipefd[1]);
return result;
case 0: { // child
close(pipefd[0]);
env = buildUserEnvironment(env);
auto sessionEnv = openSessionInternal(env);
if (!sessionEnv.has_value()) {
qCritical() << "[SessionLeader] Failed to open session. Exit now.";
_exit(1);
}
env = *sessionEnv;
int xdgId = env.value(QStringLiteral("XDG_SESSION_ID")).toInt();
if (xdgId <= 0) {
qCritical() << "[SessionLeader] Invalid XDG_SESSION_ID from pam_open_session()";
_exit(1);
}
write(pipefd[1], &xdgId, sizeof(int));
UserSession session(this);
session.setProcessEnvironment(env);
session.start(command, type, cookie);
if (!session.waitForStarted()) {
qCritical() << "[SessionLeader] Failed to start session process. Exit now.";
_exit(1);
}
qint64 pid = session.processId();
write(pipefd[1], &pid, sizeof(qint64));
qInfo() << "[SessionLeader] Session started with PID" << pid;
session.waitForFinished(-1);
if (session.exitStatus() == QProcess::CrashExit) {
qCritical() << "[SessionLeader] Session process crashed. Exit now.";
_exit(1);
}
_exit(session.exitCode());
}
default: { // parent
close(pipefd[1]);
result.readFd = pipefd[0];
if (read(pipefd[0], &result.xdgSessionId, sizeof(int)) < 0) {
qWarning() << "[Auth] Failed to read XDG_SESSION_ID:" << strerror(errno);
close(pipefd[0]);
result.readFd = -1;
return result;
}
if (read(pipefd[0], &result.sessionPid, sizeof(qint64)) < 0) {
qWarning() << "[Auth] Failed to read session PID:" << strerror(errno);
close(pipefd[0]);
result.readFd = -1;
return result;
}
return result;
}
}
}
```
`Auth::openSession` then becomes simpler orchestration:
```cpp
int Auth::openSession(const QString &command,
QProcessEnvironment env,
const QByteArray &cookie)
{
Q_ASSERT(authenticated);
SessionStartResult res = startSessionLeader(command, env, cookie);
if (!res.ok())
return -1;
xdgSessionId = res.xdgSessionId;
sessionPid = res.sessionPid;
utmpLogin(true);
m_notifier = new QSocketNotifier(res.readFd, QSocketNotifier::Read, this);
connect(m_notifier, &QSocketNotifier::activated, this, [this] {
m_notifier->setEnabled(false);
m_notifier->deleteLater();
Q_EMIT sessionFinished();
});
sessionOpened = true;
return xdgSessionId;
}
```
### 4. Avoid manual `m_notifier` deletion in the destructor
Since `m_notifier` already has `this` as parent, you don’t need `delete m_notifier;` in the destructor. You can just disable it when done:
```cpp
// Where you currently delete it explicitly:
connect(m_notifier, &QSocketNotifier::activated, this, [this] {
m_notifier->setEnabled(false);
m_notifier->deleteLater();
Q_EMIT sessionFinished();
});
```
Then simplify the destructor to only handle session/process teardown:
```cpp
Auth::~Auth()
{
if (sessionOpened) {
qWarning() << "Killing" << sessionPid << "and" << sessionLeaderPid;
if (sessionPid > 0)
kill(sessionPid, SIGTERM);
if (sessionLeaderPid > 0)
kill(sessionLeaderPid, SIGTERM);
closeSession();
utmpLogout();
}
if (d->handle) {
d->ret = pam_end(d->handle, d->ret);
if (d->ret != PAM_SUCCESS)
qWarning() << "[Auth] PAM handle end with error!";
}
qInfo() << "[Auth] Auth for user" << user << "ended.";
}
```
These changes preserve the new behavior (session leader process, pipe IPC, PAM env propagation) but:
- shorten `openSession` into high‑level orchestration,
- make PAM env handling and passwd env building reusable and testable, and
- reduce destructor responsibilities by relying on QObject ownership for `m_notifier`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: calsys456, justforlxz The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This will work as intended.
Summary by Sourcery
Rework session management in the daemon to fix multi-user login/logout behavior and align PAM/logind handling with a dedicated session leader process.
Bug Fixes:
Enhancements:
Build: