Repository navigation
fix(daemon): move PAM transaction into a dedicated session helper - #116
Conversation
|
Skipping CI for Draft Pull Request. |
Reviewer's GuideThe PR isolates the complete PAM transaction and desktop-session startup in a newly installed, exec-launched single-threaded ddm-helper, with the daemon communicating through a bounded framed socket protocol and retaining responsibility for orchestration, VT switching, utmp bookkeeping, and session state. Sequence diagram for authentication and session startup via ddm-helpersequenceDiagram
participant D as ddm daemon
participant H as ddm-helper
participant P as PAM
participant S as UserSession
D->>D: ensureHelper()
D->>H: fork+exec ddm-helper(socketFd)
D->>H: sendRequest(Authenticate)
H->>P: pam_start()
H->>P: pam_authenticate()
H->>P: pam_acct_mgmt()
P-->>H: authentication result
H-->>D: AuthResult
D->>H: sendRequest(OpenSession)
H->>P: pam_setcred()
H->>P: pam_open_session()
P-->>H: session environment
H->>S: start()
S-->>H: process PID
H-->>D: SessionResult(XDG_SESSION_ID, PID)
Flow diagram for PAM session isolation and cleanupflowchart TD
Start[Daemon receives login request] --> Spawn[Spawn exec-launched ddm-helper]
Spawn --> Auth[Helper performs PAM authentication]
Auth --> Open[Helper performs pam_open_session]
Open --> ForkSafe[ PAM module may fork safely in isolated process ]
ForkSafe --> Run[Helper starts desktop session]
Run --> Exit{Desktop session exits}
Exit --> Cleanup["Helper calls pam_close_session\nand pam_setcred(PAM_DELETE_CRED)"]
Cleanup --> Notify[Helper notifies daemon and exits]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
🟡 Changes recommended
PAM cleanup, callback lifetime, environment precedence, session orphaning, and logout accounting have correctness issues. / PAM 清理、回调生命周期、环境优先级、会话孤儿进程及注销记录存在正确性问题。
5 open findings
What changed in this PR
Moves PAM authentication and session management into an isolated helper process to avoid daemon deadlocks.
将 PAM 认证和会话管理迁移至独立 helper 进程，以避免 daemon 死锁。
Changes:
- Adds framed daemon/helper IPC and PAM lifecycle handling. / 新增 daemon/helper 帧式 IPC 与 PAM 生命周期处理。
- Decouples
UserSessionfromAuth. / 将UserSession与Auth解耦。 - Builds and installs
ddm-helper. / 构建并安装ddm-helper。
| File | Description |
|---|---|
src/helper/HelperApp.h |
Defines helper protocol and state. / 定义 helper 协议与状态。 |
src/helper/HelperApp.cpp |
Implements PAM and session execution. / 实现 PAM 与会话执行。 |
src/helper/CMakeLists.txt |
Builds and installs the helper. / 构建并安装 helper。 |
src/daemon/UserSession.h |
Adds explicit session properties. / 新增显式会话属性。 |
src/daemon/UserSession.cpp |
Removes dependency on Auth. / 移除对 Auth 的依赖。 |
src/daemon/Auth.h |
Declares helper IPC management. / 声明 helper IPC 管理。 |
src/daemon/Auth.cpp |
Delegates authentication and sessions. / 委托认证与会话处理。 |
src/CMakeLists.txt |
Includes the helper target. / 引入 helper 目标。 |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e56a62c to
5046516
Compare
|
完整时间线 ┌────────────┬───────────────────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ ⭐ 答案：4d68247 是「砍掉 helper」的那次提交 commit 4d68247
它做的事：
动机是"简化代码"，且提交信息里自己都承认： │ Note that the function of starting sessions other than treeland will temporarily not work after this commit... 也就是说，这次"简化"把 sddm 原本正确的独立 helper 架构改坏了 —— 正是这个合并动作，让 PAM 事务跑进了 Qt 多线程 daemon 里，埋下了 gnome-keyring fork+exit 死锁的雷。 结论 这次 PR #116（346ba8f）本质上是对 4d68247 那次"简化"的回退——把 2025 年 12 月被砍掉的独立 helper 架构重新恢复回来。所以：
|
There was a problem hiding this comment.
🟡 Changes recommended
Session-exit races, prolonged password retention, and mismatched shutdown timeouts can leak PAM or desktop-session resources. / 会话退出竞态、密码长期驻留及关闭超时不匹配可能泄漏 PAM 或桌面会话资源。
2 open findings
2 resolved since last review
🧠 Review effort: Balanced
The full PAM transaction (authenticate + open_session) now runs in a standalone single-threaded ddm-helper process spawned via fork+exec. gnome-keyring's PAM module forks inside pam_open_session and calls exit() in the grandchild; doing that inside the multithreaded Qt daemon triggered inherited atexit handlers and deadlocked on QThread::wait(). 将 PAM 完整流程（认证+会话）迁移到独立的单线程 ddm-helper 进程， 该进程通过 fork+exec 启动，不建立 DBus 连接。gnome-keyring 的 PAM 模块会在 pam_open_session 中 fork 子进程并调用 exit()，在多线程的 Qt 守护进程中会触发继承的 atexit 处理器并死锁在 QThread::wait()。 PMS: BUG-379365 Log: 将 PAM 认证与会话迁移到独立单线程 helper 进程 Influence: 修复图形登录时 gnome-keyring 触发 Qt 多线程 fork 死锁的问题
Move utmp/wtmp/btmp bookkeeping from the daemon into ddm-helper, matching SDDM's design where accounting follows the PAM session lifecycle. 将 utmp/wtmp 记账从 daemon 迁移到 ddm-helper，对齐 SDDM 设计， 使记账随 PAM 会话生命周期进行。 Log: 将 utmp/wtmp 记账下沉到 helper Influence: 注销记录不再依赖 daemon 侧 sessionOpened 状态时序，避免会话结束后遗留陈旧的 utmp/wtmp 记录。
deepin pr auto reviewAI 代码审查报告
总体评价
总体评价结论: 代码审查通过。本次提交将 PAM 事务从多线程 daemon 进程迁移到独立的单线程 helper 进程（ddm-helper），有效解决了 gnome-keyring PAM 模块在 漏洞对比统计: 新增漏洞 0 个，减少漏洞 0 个，持平 0 个 提交目的分析本次 PR 的核心目的是修复 DDM（Deepin Display Manager）登录死锁问题：
维度1：语法逻辑（23/25）✓
问题列表
正面评价
维度2：代码质量（23/25）✓
问题列表
正面评价
维度3：代码性能（19/20）✓
问题列表
正面评价
维度4：代码安全（28/30）✓
安全漏洞清单: 无 漏洞对比统计: 新增漏洞 0 个，减少漏洞 0 个，持平 0 个 安全实践评估良好安全实践:
安全建议（非漏洞）:
改进建议建议代码示例// 1. handleAuthenticate() 中擦除局部密码变量
void HelperApp::handleAuthenticate(QDataStream &in)
{
QString user, pamService;
QByteArray secret;
in >> user >> pamService >> secret;
const bool ok = doAuthenticate(user, pamService, secret);
if (!ok)
utmpLogin(false);
// 擦除局部密码变量
secret.fill(0);
// 使用 QDataStream 一致序列化
QByteArray payload;
{
QDataStream ds(&payload, QIODevice::WriteOnly);
ds << ok;
}
sendMsg(AuthResult, payload);
}
// 2. sendRequest() 添加超时机制（可选）
bool Auth::sendRequest(quint32 op, const QByteArray &payload, QByteArray &reply)
{
// ... 现有写入逻辑 ...
// 使用 poll() 添加超时
struct pollfd pfd;
pfd.fd = d->socketFd;
pfd.events = POLLIN;
const int timeout_ms = 30000; // 30 秒超时
for (;;) {
int ret = ::poll(&pfd, 1, timeout_ms);
if (ret == 0) {
qWarning() << "[Auth] helper response timeout";
return false;
}
if (ret < 0) {
if (errno == EINTR) continue;
return false;
}
// ... 现有读取和帧解析逻辑 ...
}
}审查清单
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Groveer, wineee 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 |
DDM 调研报告
1. 项目概述
DDM（Deepin Display Manager）是 deepin 桌面发行版的显示管理器，fork 自 SDDM。
org.freedesktop.login1）与上游 SDDM 相比，ddm 最大的差异在于：greeter（登录界面）不再是一个独立的 QtQuick 进程，而是由 Treeland 合成器直接承载，ddm 只保留守护进程（daemon），通过
QLocalSocket与 Treeland 通信。2. 进程模型
关键设计：
ddm-helper是本次死锁修复新增的独立二进制，负责完整 PAM 事务，与 daemon 通过socketpair通信。daemon 保持 Qt 多线程（QDBus），而 helper 保持单线程、无 DBus，二者各司其职。3. 源码结构
构建产物：
/usr/bin/ddm（daemon）/usr/libexec/ddm-helper（会话助手）libddm-common.so（共享库）4. 核心模块详解
4.1 DaemonApp — 守护进程装配
DaemonApp继承QCoreApplication，是 daemon 的单例入口（daemonApp宏），负责实例化并持有所有管理器：4.2 DisplayManager — DBus 接口
实现
org.freedesktop.DisplayManager（以及 deepin 扩展org.deepin.DisplayManager），对外暴露Seats/Sessions/LastSession/AuthInfo等属性。session 对象通过Qt DBus adaptor从 XML 自动生成。4.3 SeatManager + Display — seat/display 生命周期
SeatManager监听 logind 的 seat 增删，为每个图形 seat 创建一个Display。Display代表一个 seat 的完整登录上下文：fetchAvailableVt，默认DDM_INITIAL_VT=1）SocketServer与 greeter 通信auths列表（进行中的认证对象）4.4 SocketServer + Messages — greeter 通信协议
SocketServer在/run/ddm下创建QLocalServer，与 Treeland 中的 greeter 通信。消息用SocketWriter（QDataStream）序列化。消息枚举（
Messages.h）：Login/Logout/Lock/Unlock/PowerOff/Reboot/ ...ShowGreeter/LoginFailed/Capabilities/HostName/UserLoggedIn/ ...4.5 Auth — 认证编排（daemon 侧）
Auth是登录流程的核心编排对象，对外接口保持稳定：本次重构后，
Auth内部不再直接调用 PAM，而是通过AuthPrivate持有QProcess（helper）和socketpair，用sendRequest(op, payload, reply)与 helper 通信。4.6 HelperApp — 会话助手（helper 侧）
ddm-helper是单线程QCoreApplication，构造时：argv[1]读取继承的 socket fdO_NONBLOCK（避免与阻塞在read()的 daemon 互相死锁）QSocketNotifier监听数据处理两条指令：
Authenticatepam_start→pam_authenticate→pam_acct_mgmtOpenSessionpam_setcred→pam_open_session→pam_getenvlist→ 启动UserSession完整 PAM 会话链：
会话结束（
onSessionFinished）时执行pam_close_session→pam_setcred(PAM_DELETE_CRED)→pam_end。4.7 UserSession — 用户会话进程启动
UserSession继承QProcess，通过childModifier()（在子进程中执行）完成：setuid降权到目标用户setsid+ 设置进程组exec桌面会话命令5. 登录流程时序
会话结束：
6. 关键问题调研：多线程 fork 死锁
6.1 现象
用户报告：更新后点击登录无响应（ddm 图形登录死锁）。gdb 附加后确认卡死点：
6.2 根因链
本质：glibc 手册明确规定，多线程进程 fork 后，子进程在调用
exec之前只能使用 async-signal-safe 函数。exit()会遍历 atexit 处理器和全局析构，在多线程 fork 的子进程中是不安全的。gnome-keyring 的 PAM 模块（pam_gnome_keyring.so）在 fork 的孙进程里调用exit()，触发 Qt 的线程清理死锁。为何 lightdm 无此问题：lightdm 是 GLib 程序，其 helper 与主进程隔离良好；而 ddm 是 Qt6 程序，QDBus 线程的存在使 fork 后
exit()必然踩雷。6.3 解决方案（两条互补路线）
路线一：ddm 架构对齐（已合并，commit
346ba8f）仿 sddm 的 helper 架构，把完整 PAM 事务从 daemon 迁移到独立单线程
ddm-helper进程：fork + exec启动 helper（不再在 daemon 内fork后调 PAM）QCoreApplication，从不建立 DBus 连接，保持单线程fork + exit时，触发的是 helper 自己的 atexit（无后台线程），安全路线二：gnome-keyring 源码修复（独立仓库）
将 gnome-keyring PAM 模块 6 处 fork 子进程里的
exit()改为_exit()，从源头消除「多线程 fork 后调 exit()」这一 glibc 禁止行为，对所有 Qt 显示管理器通用安全。6.4 死锁问题对架构设计的启示
O_NONBLOCK，否则 daemon 阻塞在read()等回复、helper 阻塞在第二次read()，形成新的死锁。7. daemon ↔ helper 通信协议
7.1 传输层
socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC)sv[0]，helper 持sv[1]（通过argv[1]传 fd 号，exec 前清除FD_CLOEXEC）O_NONBLOCK7.2 帧格式
所有帧用
QDataStream（big-endian）序列化：AuthenticateOpenSessionAuthResultSessionResultSessionFinishedSummary by Sourcery
Move PAM authentication and user-session management into an isolated helper process to avoid multithreaded fork deadlocks in the daemon.
New Features:
Bug Fixes:
Enhancements:
Build: