Skip to content

fix: stabilize Treeland VT session switching#94

Open
zccrs wants to merge 2 commits into
linuxdeepin:masterfrom
zccrs:vt-treeland-dde-seatd
Open

fix: stabilize Treeland VT session switching#94
zccrs wants to merge 2 commits into
linuxdeepin:masterfrom
zccrs:vt-treeland-dde-seatd

Conversation

@zccrs
Copy link
Copy Markdown
Member

@zccrs zccrs commented May 27, 2026

Summary

  • integrate standalone dde-seatd service and inject SEATD_SOCK for treeland.service
  • run Treeland PAM sessions through a clean helper process to preserve keyring unlock
  • fix managed/unmanaged VT switching and add wlroots VM validation tools

Validation

  • cmake --build build
  • ctest --test-dir build --output-on-failure
  • git diff --check
  • product VM validation: alice tty2, bob tty3, unmanaged tty6 pause/resume

Related

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @zccrs, your pull request is larger than the review limit of 150000 diff characters

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zccrs

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@zccrs zccrs force-pushed the vt-treeland-dde-seatd branch 8 times, most recently from 1dcc5df to fec0370 Compare May 28, 2026 06:53
Depend on dde-seatd.service from the dde-seatd package and point Treeland at /run/dde-seatd.sock. This keeps DDM from installing its own seatd service while preserving the product runtime wiring.
@zccrs zccrs force-pushed the vt-treeland-dde-seatd branch from fec0370 to 235d3d6 Compare May 28, 2026 10:17
Connect DDM to the dde-seatd control socket and register Treeland user VTs with the compositor owner pidfd. This keeps DDM focused on session policy while dde-seatd owns grouped VT switching and seat client lifetime.
@zccrs zccrs force-pushed the vt-treeland-dde-seatd branch from 235d3d6 to 64abec6 Compare May 28, 2026 10:29
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

这份代码变更非常庞大,主要涉及将显示管理器 (ddm) 的 seat 管理从原生的 seatd 迁移到自定义的 dde-seatd,重构了 Treeland 会话的 VT(虚拟终端)管理机制,并引入了一个“Session Helper”子进程架构来处理 PAM 和会话的创建。

以下是对该 diff 的详细代码审查,涵盖语法逻辑、代码质量、性能和安全性四个方面:

一、 语法与逻辑

  1. 管道读取的竞态条件与阻塞问题 (Auth.cpp)

    • 问题readAllFromFdreadAllFromFd 函数假设能够一直读到 EOF (bytes == 0)。但在 openSessionInHelperProcess 中,父进程在写入请求后关闭了写端,子进程在写入响应后关闭了 STDOUT_FILENO。这虽然能保证父进程的 readAllFromFd 能收到 EOF,但如果 Helper 进程崩溃或挂起,父进程会永久阻塞在这里,导致整个 DM 主进程卡死。
    • 建议:使用 pollselect 为读取操作设置超时机制,或者使用异步 I/O (如 QSocketNotifier)。
  2. pipe 读取不完整的风险 (Auth.cpp)

    • 问题:在 Auth::openSession 的父进程中,代码改进了对 read 返回值的检查,但 read 仍然可能因为信号中断(EINTR)或内核缓冲区原因返回部分数据(例如只读了 2 字节的 int)。
    • 建议:封装一个 readExact 函数,在循环中处理 EINTR 并确保读取完整的 sizeof(int)sizeof(qint64)
  3. QDataStream 跨进程通信的版本兼容性 (Auth.cpp)

    • 问题serializeRequestdeserializeResponse 使用了 QDataStream。Qt 的 QDataStream 格式在不同主版本之间可能不兼容。
    • 建议:显式设置数据流版本,例如:stream.setVersion(QDataStream::Qt_5_15);,以防未来 Qt 升级导致通信失败。
  4. 信号处理器的连接上下文 (VirtualTerminal.cpp)

    • 问题QObject::connect(daemonApp->signalHandler(), &SignalHandler::customSignalReceived, daemonApp->signalHandler(), onVtSignal); 将信号连接到了 daemonApp->signalHandler() 所在的线程。如果信号是在 UI 线程发出,而 VT 操作必须在主线程执行,需确保上下文正确。
  5. strncpy 缺少手动添加 \0 (TreelandConnector.cpp)

    • 问题:在 connectUnixSocket 中,strncpy(addr.sun_path, path, sizeof(addr.sun_path) - 1); 如果 path 长度刚好大于等于 sizeof-1strncpy 不会自动补全末尾的 \0
    • 建议:添加 addr.sun_path[sizeof(addr.sun_path) - 1] = '\0';

二、 代码质量

  1. 重复的管道创建与清理代码 (Auth.cpp)

    • 问题openSessionInHelperProcess 中连续创建了 3 个管道,每个创建失败都有大量的 closereturn 重复代码,非常冗余且容易漏关。
    • 建议:使用 RAII(如 QScopeGuard 或自定义 FileDescriptor 类)管理文件描述符,或者提取一个 createPipe(int[2]) 辅助函数来统一错误处理。
  2. 魔法数字与硬编码 (TreelandConnector.cpp, Display.cpp)

    • 问题
      • fetchAvailableUserVt 中硬编码了 64 作为最大 VT 数量。
      • handleControlSocket 中硬编码了 4096 作为最大消息大小。
    • 建议:将这些定义为具名常量(constexpr int MAX_VT_COUNT = 64;),提高可读性和可维护性。
  3. childModifier 中的 C 风格内存管理 (UserSession.cpp)

    • 问题:将 new[] 替换为了 malloc,虽然是为了配合 C 函数,但多处 malloc 后没有检查返回值(虽然现代 Linux 默认启用 OOM killer,几乎不会返回 NULL,但严谨起见仍需检查)。
    • 建议:可以使用 QScopedPointer 或确保每次 malloc 都有对应的 free 且检查返回值。
  4. makeParentDirs 的递归创建逻辑 (UserSession.cpp)

    • 问题:当前的实现是逐级打断字符串并创建,逻辑略显复杂。
    • 建议:Qt 提供了 QDir().mkpath(),在 setuid 之前的父进程中使用 Qt 的方法创建目录可能更安全且简洁。如果在子进程必须用 C,当前逻辑也能工作,但需确保 filePath 绝对路径正确。

三、 代码性能

  1. Helper 进程的冷启动开销 (Auth.cpp)

    • 问题:对于 Treeland 会话,每次登录都会 fork()execl() 一个全新的 Helper 进程。fork 开销虽小,但 execl 重新加载动态链接器和初始化 Qt 环境是非常耗时的。
    • 建议:如果登录频率较高,考虑将 Helper 做成常驻的守护进程(通过 Unix Socket 通信),而不是按需启动一次性进程。
  2. QDBusInterface 的实时查询 (TreelandConnector.cpp)

    • 问题treelandMainPid() 每次被调用(即每次创建 Group VT)都会通过 DBus 同步查询 systemd 获取 MainPID。QDBusInterface::call 是同步阻塞的。
    • 建议:缓存 treelandMainPid,或者监听 systemd 的 PropertiesChanged 信号来更新 PID,避免在关键路径上进行同步 DBus 调用。
  3. QByteArray 的频繁分配 (TreelandConnector.cpp)

    • 问题handleControlSocket 每次读取 256 字节并 append 到 m_controlBuffer,然后 remove(0, messageSize) 会导致内存拷贝(memmove)。
    • 建议:对于高频的 Socket 通信,可以考虑使用环形缓冲区,或者使用 QByteArray::mid() 结合 swap 来避免频繁的内存移动。

四、 代码安全

  1. ⚠️ 密码明文在内存中的残留 (Auth.cpp)

    • 问题:在 openSessionInHelperProcess 中,secret.fill('\0'); 清除了 request.secret。但是,SessionHelperRequest 结构体在反序列化时会在 runSessionHelper 的栈/堆上产生新的副本,且 QDataStream 内部的缓冲区也可能持有明文密码。
    • 建议:确保 Helper 进程中 request->secret 使用后也被 fill('\0') 清除。同时,QByteArray 在析构时不保证擦除内存,建议自定义安全擦除的 ByteArray 类。
  2. ⚠️ 子进程环境变量泄露 (Auth.cpp)

    • 问题:在 openSessionInHelperProcesscase 0: 中,使用了 setenv("DDM_SESSION_HELPER_LIFECYCLE_FD", ...)。由于 execl 继承当前环境,这个环境变量会泄露给 ddm --session-helper 进程及其所有子进程。
    • 建议:如果 lifecycleFd 仅用于 Helper 自身,应在 Helper 读取后立即 unsetenv("DDM_SESSION_HELPER_LIFECYCLE_FD"),防止其传递给最终的 User Session。
  3. ⚠️ Socket 权限与路径安全 (TreelandConnector.cpp)

    • 问题connectUnixSocket 连接到 /run/dde-seatd-control.sock。如果该 socket 权限配置不当(例如 other 可写),任意用户进程都可以发送 ControlCreateGroupVt 伪造请求,造成提权或 VT 劫持。
    • 建议:确保 dde-seatd 创建的 socket 权限严格限制为 0660 且属主为 root:ddm 组。同时,dde-seatd 应该校验连接该 socket 的进程 UID。
  4. ⚠️ 文件描述符泄露风险 (Auth.cpp)

    • 问题fork 之后,子进程继承了父进程的所有文件描述符(包括其他客户端的 socket 等)。虽然代码中关闭了 pipe,但未关闭其他无关 FD。
    • 建议:在父进程中维护一个需要保留的 FD 集合,子进程创建后应遍历并关闭所有不在集合中的 FD(类似 closefrom),防止资源泄露和权限越权。
  5. chown 的符号链接竞争 (TOCTOU) (Display.cpp)

    • 问题chown(qPrintable(authSocket), ...) 中,如果 authSocket 是一个符号链接,攻击者可能在其创建到 chown 之间替换该链接,导致修改错误文件的所有权。
    • 建议:使用基于文件描述符的操作,即先 open 获取 FD,然后 fchown(fd, ...),确保操作的是同一个文件。

总结

该 PR 的架构调整方向是合理的,特别是将 PAM 和 Session 开启隔离到 Helper 进程,有效防止了主进程阻塞。然而,安全性和健壮性是最大的隐患:密码在 IPC 传输过程中的残留清理、Helper 进程的 FD 泄露、以及同步阻塞调用可能导致的 DM 卡死问题,需要在合并前重点修复或确认。建议优先解决管道读取超时/阻塞问题以及环境变量泄露问题。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants