Skip to content

fix(touchscreen-dialog): fix crash when monitor list changes#424

Open
electricface wants to merge 1 commit intolinuxdeepin:masterfrom
electricface:swt/fix-bug315115
Open

fix(touchscreen-dialog): fix crash when monitor list changes#424
electricface wants to merge 1 commit intolinuxdeepin:masterfrom
electricface:swt/fix-bug315115

Conversation

@electricface
Copy link
Member

@electricface electricface commented Mar 8, 2026

When m_listCombo->clear() is called in onMonitorChanged(), Qt
emits currentIndexChanged(-1), triggering markDisplay(-1) and
causing an out-of-bounds crash on m_monitors[-1].

Add a boundary check in markDisplay() to return early on invalid index.

Log: fix crash when monitor list changes
pms: BUG-315115

Summary by Sourcery

Prevent crashes in the touchscreen settings dialog when the monitor list changes and the selected monitor index becomes invalid.

Bug Fixes:

  • Guard touchscreen display highlighting against out-of-bounds monitor indices to avoid crashes when the combo box selection is cleared or invalidated.

Enhancements:

  • Update SPDX file copyright years for the touchscreen settings source file.

When `m_listCombo->clear()` is called in `onMonitorChanged()`, Qt
emits `currentIndexChanged(-1)`, triggering `markDisplay(-1)` and
causing an out-of-bounds crash on `m_monitors[-1]`.

Add a boundary check in `markDisplay()` to return early on invalid index.

Log: fix crash when monitor list changes
pms: BUG-315115
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: electricface

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

@sourcery-ai
Copy link

sourcery-ai bot commented Mar 8, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adds bounds checking to TouchscreenSetting::markDisplay to prevent out-of-bounds access when the monitor list changes and updates the SPDX copyright year range.

Sequence diagram for monitor list change causing markDisplay invocation

sequenceDiagram
    participant Qt as QtFramework
    participant Combo as MonitorListComboBox
    participant TS as TouchscreenSetting

    Qt->>Combo: monitorsChanged()
    Combo->>Combo: clear()
    Combo-->>TS: currentIndexChanged(-1)
    TS->>TS: markDisplay(-1)
    TS->>TS: index < 0 or index >= m_monitors.size()
    TS-->>TS: return (no access to m_monitors)
Loading

Class diagram for updated TouchscreenSetting bounds checking

classDiagram
    class TouchscreenSetting {
        - QList~QRect*~ m_monitors
        + bool monitorsIsIntersect() const
        + void markDisplay(int index)
    }

    class QRect

    TouchscreenSetting --> QRect : uses in m_monitors
Loading

File-Level Changes

Change Details Files
Prevent out-of-bounds access in markDisplay when an invalid monitor index is passed.
  • Add an early-return guard in markDisplay when index is negative or greater than or equal to the number of monitors.
  • Rely on the guard to avoid accessing m_monitors with an invalid index when the combo box emits currentIndexChanged(-1) after being cleared.
dde-touchscreen-dialog/src/touchscreensetting.cpp
Update the SPDX copyright header to extend the year range.
  • Change the SPDX-FileCopyrightText range from 2019 - 2022 to 2019 - 2026.
dde-touchscreen-dialog/src/touchscreensetting.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

deepin pr auto review

这段代码的修改主要涉及版权年份更新和边界检查的增加。以下是对这段 diff 的详细审查和改进建议:

1. 语法逻辑审查

版权年份更新:

// SPDX-FileCopyrightText: 2019 - 2026 UnionTech Software Technology Co., Ltd.
  • 问题:将版权年份更新到 2026 年是不恰当的。版权年份通常只更新到当前年份,或者项目预期的结束年份(如果项目有明确的结束时间)。提前设置到未来年份可能会引起法律或合规性问题。
  • 建议:将年份更新为当前年份(如 2023 或 2024)。

边界检查:

void TouchscreenSetting::markDisplay(int index)
{
    if (index < 0 || index >= m_monitors.size())
        return;
  • 优点:增加了对 index 的边界检查,防止数组越界访问。
  • 问题:当 index 越界时,函数直接返回,没有提供任何错误反馈(如日志、错误码或异常)。这可能会让调用者难以调试问题。
  • 建议:可以添加日志记录或返回错误状态。

2. 代码质量审查

边界检查的改进:

void TouchscreenSetting::markDisplay(int index)
{
    if (index < 0 || index >= m_monitors.size()) {
        qWarning() << "Invalid monitor index:" << index << "Valid range: 0 -" << m_monitors.size() - 1;
        return;
    }
  • 改进:添加日志记录,帮助调试。

函数命名和注释:

  • markDisplay 的命名较为模糊,建议改为 markMonitorDisplayhighlightMonitor 以更清晰地表达其功能。
  • 函数注释可以补充说明 index 的有效范围和越界时的行为。

3. 代码性能审查

  • 边界检查m_monitors.size() 是一个 O(1) 操作,因此性能影响可以忽略。
  • 设备像素比计算qApp->devicePixelRatio() 可能是一个相对耗时的操作,但考虑到 markDisplay 可能不会被频繁调用,性能影响可以接受。如果性能敏感,可以缓存 devicePixelRatio

4. 代码安全审查

  • 数组越界:新增的边界检查有效防止了数组越界访问,提升了安全性。
  • 空指针检查:如果 m_monitors[index] 可能返回空指针(取决于 m_monitors 的实现),建议增加空指针检查:
    if (!m_monitors[index]) {
        qWarning() << "Monitor at index" << index << "is null";
        return;
    }

5. 改进后的代码示例

// SPDX-FileCopyrightText: 2019 - 2024 UnionTech Software Technology Co., Ltd.
//
// SPDX-License-Identifier: GPL-3.0-or-later

void TouchscreenSetting::markMonitorDisplay(int index)
{
    // 边界检查
    if (index < 0 || index >= m_monitors.size()) {
        qWarning() << "Invalid monitor index:" << index << "Valid range: 0 -" << m_monitors.size() - 1;
        return;
    }

    // 空指针检查(如果需要)
    if (!m_monitors[index]) {
        qWarning() << "Monitor at index" << index << "is null";
        return;
    }

    // 考虑设备像素比,确保示意框大小与实际显示器大小一致
    const auto ratio = qApp->devicePixelRatio();
    QRect displayRect(m_monitors[index]->x(), m_monitors[index]->y(),
                      m_monitors[index]->width() * ratio,
                      m_monitors[index]->height() * ratio);
    // 其余逻辑...
}

6. 总结

  • 优点:新增的边界检查有效提升了代码的健壮性。
  • 缺点:版权年份更新到未来年份不合适,且缺少错误反馈。
  • 建议
    1. 将版权年份更新为当前年份。
    2. 添加日志记录或错误反馈机制。
    3. 考虑空指针检查(如果适用)。
    4. 优化函数命名和注释以提升可读性。

Copy link

@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.

Hey - I've reviewed your changes and they look great!


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.

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