Skip to content

fix: use sigkill for wayland killClient()#1631

Merged
BLumia merged 1 commit into
linuxdeepin:masterfrom
BLumia:pms-364201
Jun 11, 2026
Merged

fix: use sigkill for wayland killClient()#1631
BLumia merged 1 commit into
linuxdeepin:masterfrom
BLumia:pms-364201

Conversation

@BLumia

@BLumia BLumia commented Jun 11, 2026

Copy link
Copy Markdown
Member

之前 wayland 下强行停止进程的实现和关闭窗口一样,可能会导致一些程序
无法被强行停止. 现在改用使用 pid 并发送 SIGKILL 的方式强行停止进程

PMS: BUG-364201

Summary by Sourcery

Bug Fixes:

  • Ensure Wayland tasks can always be forcibly stopped by sending SIGKILL to the associated process when killClient() is invoked.

之前 wayland 下强行停止进程的实现和关闭窗口一样,可能会导致一些程序
无法被强行停止. 现在改用使用 pid 并发送 SIGKILL 的方式强行停止进程

PMS: BUG-364201
Log:
@BLumia BLumia requested review from 18202781743 and tsic404 June 11, 2026 07:48
@sourcery-ai

sourcery-ai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

This PR changes the Wayland task manager’s killClient() implementation to terminate clients by sending SIGKILL to their PID instead of requesting a normal close, improving reliability of forced termination, and adds logging when the PID is unavailable.

Sequence diagram for updated killClient forced termination

sequenceDiagram
    actor User
    participant TreeLandWindow
    participant ForeignToplevelHandle
    participant OS

    User->>TreeLandWindow: killClient()
    TreeLandWindow->>ForeignToplevelHandle: pid()
    ForeignToplevelHandle-->>TreeLandWindow: pid
    alt [pid <= 0]
        TreeLandWindow->>TreeLandWindow: waylandwindowLog()
        TreeLandWindow->>TreeLandWindow: qCWarning(...)
    else [pid > 0]
        TreeLandWindow->>OS: kill(pid, SIGKILL)
    end
Loading

File-Level Changes

Change Details Files
Change killClient() to send SIGKILL to the window’s PID instead of invoking a graceful close on the Wayland foreign toplevel handle, with added logging when PID lookup fails.
  • Include <signal.h> to access kill() and SIGKILL.
  • Retrieve the client PID from the foreign toplevel handle before attempting to kill.
  • Guard against invalid or missing PIDs by checking pid <= 0 and logging a warning with appid and id when this occurs.
  • Use ::kill(pid, SIGKILL) to forcefully terminate the client process instead of calling close() on the handle.
panels/dock/taskmanager/treelandwindow.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

这段代码实现了终止进程的功能,我来分析一下其安全性、逻辑和潜在改进点:

  1. 安全性问题:

    • 使用SIGKILL信号强制终止进程可能会导致数据丢失,因为进程没有机会进行清理工作
    • 没有权限检查,任何用户都可以终止任何进程,这可能带来安全风险
    • 没有对特殊进程(如系统关键进程)的保护机制
  2. 代码逻辑:

    • 代码首先获取进程ID(pid),然后检查pid是否有效
    • 如果pid无效,会记录警告日志并返回
    • 最后使用kill系统调用发送SIGKILL信号
  3. 改进建议:

void TreeLandWindow::killClient()
{
    const auto pid = m_foreignToplevelHandle->pid();
    if (pid <= 0) {
        qCWarning(waylandwindowLog()) << "Cannot find pid of window" 
            << m_foreignToplevelHandle->appid() << m_foreignToplevelHandle->id();
        return;
    }

    // 检查是否为系统进程
    if (pid < 1000) {  // 通常系统进程ID小于1000
        qCWarning(waylandwindowLog()) << "Attempt to kill system process with pid:" << pid;
        return;
    }

    // 先尝试使用SIGTERM优雅终止
    if (::kill(pid, SIGTERM) == 0) {
        // 可以设置一个定时器,如果进程在超时后仍在运行,再使用SIGKILL
        qCDebug(waylandwindowLog()) << "Sent SIGTERM to process" << pid;
    } else {
        qCWarning(waylandwindowLog()) << "Failed to send SIGTERM to process" << pid 
            << "Error:" << strerror(errno);
        return;
    }
}
  1. 其他建议:

    • 添加权限检查,确保只有有权限的用户才能终止进程
    • 考虑添加确认对话框,防止用户误操作
    • 可以添加进程终止后的清理逻辑
    • 建议使用更安全的进程终止方式,先尝试SIGTERM,等待一段时间后再使用SIGKILL
    • 添加更详细的日志记录,包括进程信息和终止原因
  2. 性能考虑:

    • 当前代码的性能已经足够好,主要是系统调用,没有明显的性能瓶颈
    • 如果需要频繁调用killClient,可以考虑添加进程状态缓存
  3. 错误处理:

    • 当前代码只处理了pid无效的情况
    • 建议添加对kill系统调用返回值的检查,并处理可能的错误
    • 添加errno的错误信息输出,有助于调试

这些改进可以提高代码的安全性、可靠性和用户体验,同时更好地处理各种边界情况。

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • Consider checking the return value of ::kill and logging a warning (including errno) when it fails so that issues like permission problems or stale PIDs are easier to diagnose.
  • If there’s any chance m_foreignToplevelHandle can become invalid or null by the time killClient() is called, it would be safer to guard against this before dereferencing it for pid(), appid() and id().
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider checking the return value of `::kill` and logging a warning (including `errno`) when it fails so that issues like permission problems or stale PIDs are easier to diagnose.
- If there’s any chance `m_foreignToplevelHandle` can become invalid or null by the time `killClient()` is called, it would be safer to guard against this before dereferencing it for `pid()`, `appid()` and `id()`.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743, BLumia, tsic404, tsix404

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

@BLumia BLumia merged commit 0faf4a7 into linuxdeepin:master Jun 11, 2026
10 of 12 checks passed
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.

5 participants