Skip to content

fix: guard detail "More" button click against stale list access - #732

Open
GongHeng2017 wants to merge 1 commit into
linuxdeepin:develop/eaglefrom
GongHeng2017:develop/eagle
Open

fix: guard detail "More" button click against stale list access#732
GongHeng2017 wants to merge 1 commit into
linuxdeepin:develop/eaglefrom
GongHeng2017:develop/eagle

Conversation

@GongHeng2017

@GongHeng2017 GongHeng2017 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

根因分析

扩展模式(≥2 显示器)下显示设备页经 PageInfoWidget::updateTable 选中多设备页 PageMultiInfo → PageDetail,其"更多"按钮 DetailButton(tr("More")) 的槽 PageDetail::slotBtnClickedsrc/Page/PageDetail.cpp:393)用 foreach+breakm_ListDetailButton 查按钮下标,未校验是否命中m_ListTextBrowser[index](:410)取 TextBrowser;命中失败时 index==size() 越界读得野指针 → browser->updateShowOtherInfo() 解引用 → SEGV。

触发机理:刷新(启动 DBus refreshInfo+singleShot(2000) / 右键刷新 / 切页 / 扩展模式显示配置变化)走 MainWindow::refresh → DeviceWidget::clear → PageDetail::clearWidgetclearWidgetae0fff23(Bug-162167) 起为「先清空成员链表、再对副本 deleteLater」——成员链表已空、旧按钮延迟销毁仍存活的时间窗内,其 clicked 派发到 slotBtnClicked → 空链表越界 → SEGV。与 core.txt 回溯 QAbstractButton::clicked → activate → 3 帧自身代码 → SEGV 及"仅扩展模式 / 闪退一次 / 概率性"现象一致。

关键证据:

  • PageDetail.cpp:399-404,410 — 查找无命中校验 + 越界 m_ListTextBrowser[index]
  • PageInfoWidget.cpp:56-76lst.size()>=2 且非 BIOS → PageMultiInfo/PageDetail(扩展模式专属路径,区别于单屏 PageSingleInfo/DetailTreeView)。
  • log/core.txt#4 QAbstractButton::clicked → #3 activate → #0..#2 deepin-devicemanager 自身

修复方案

  1. 主修复slotBtnClickedm_ListDetailButton.indexOf(button) 替换 foreach+break,命中且 index < m_ListTextBrowser.size() 才继续,否则 return(忽略过期/空链表点击)。
  2. 加固clearWidgetdeleteLater()DetailButtondisconnectclicked 信号,从根源阻断「链表已空、旧按钮延迟销毁仍派发点击」的时间窗。
  3. 同步修复showDeviceInfom_ListDetailSeperator[lstInfo.size()-1] 同类越界(if(!device) continue 致链表短于 lstInfo.size()),改用 m_ListDetailSeperator.last() 并判空。

改动安全评估

低风险。三处改动均在 PageDetail.cpp,无函数签名变更、无公开 API 变更、无删除公开成员。slotBtnClickedPageDetail.h 声明 + ut_pagedetail.cpp:147 直接调用(无 sender、空链表场景):保留 if(!button) return 守卫在前,行为不变、既有 UT 仍通过。clearWidget/showDeviceInfo 调用者(PageMultiInfo)无感知。正常路径行为不变,仅修正「过期点击越界」「null 设备越界」两条异常路径(由崩溃→安全忽略)。

关联

Summary by Sourcery

Guard PageDetail "More" button handling against stale widgets to prevent crashes when refreshing device info in multi-display mode.

Bug Fixes:

  • Prevent out-of-bounds access in PageDetail::slotBtnClicked by validating the clicked button index against the text browser list.
  • Avoid separator list index overflow in showDeviceInfo when some device entries are skipped.
  • Eliminate crashes caused by delayed deletion of DetailButton instances by disconnecting their clicked signals before clearing.

1. 修复 PageDetail::slotBtnClicked 未校验按钮查找命中即按下标访问 m_ListTextBrowser 的越界崩溃;
2. clearWidget 在 deleteLater 旧 DetailButton 前先 disconnect 其 clicked 信号,阻断刷新重建期间旧按钮延迟销毁仍派发点击的时间窗;
3. 修复 showDeviceInfo 中 m_ListDetailSeperator[lstInfo.size()-1] 同类越界(if(!device) continue 致链表短于 lstInfo.size()),改用 last() 并判空;

=====================================

1. fix out-of-bounds access in PageDetail::slotBtnClicked where the button lookup result was not validated before indexing m_ListTextBrowser;
2. disconnect DetailButton::clicked before deleteLater in clearWidget to block stale clicks delivered during the deferred-deletion window after a refresh rebuild;
3. fix parallel out-of-bounds in showDeviceInfo where m_ListDetailSeperator[lstInfo.size()-1] could exceed list length when null devices were skipped, now uses last() with an empty check;

Log: 修复扩展模式(多显示器)下点击显示设备“更多”按钮偶发闪退问题,根因为刷新重建页面时旧按钮延迟销毁期间其点击派发到已清空的按钮列表导致越界访问,通过校验查找命中与断开旧按钮信号根治

Bug: https://pms.uniontech.com/bug-view-308175.html

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

Sorry @GongHeng2017, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: GongHeng2017

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

sourcery-ai Bot commented Aug 17, 2026

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

Reviewer's Guide

Guard PageDetail’s "More" button handling against stale widgets and list index mismatches to prevent SEGVs, and fix a related separator visibility out-of-bounds access in multi-device views.

Sequence diagram for guarded DetailButton click handling

sequenceDiagram
    actor User
    participant DetailButton
    participant PageDetail
    participant TextBrowser

    User->>DetailButton: clicked
    DetailButton->>PageDetail: slotBtnClicked
    PageDetail->>PageDetail: qobject_cast DetailButton sender
    alt [button is null]
        PageDetail-->>PageDetail: return
    else [button is non-null]
        PageDetail->>PageDetail: m_ListDetailButton.indexOf button
        alt [index < 0 or index >= m_ListTextBrowser.size]
            PageDetail-->>PageDetail: return (ignore stale click)
        else [index valid]
            PageDetail->>DetailButton: updateText
            PageDetail->>TextBrowser: updateShowOtherInfo
        end
    end
Loading

File-Level Changes

Change Details Files
Harden showDeviceInfo’s handling of detail separators to avoid out-of-bounds access when some devices are skipped.
  • Replace direct indexed access m_ListDetailSeperator[lstInfo.size()-1] with m_ListDetailSeperator.last().
  • Add an is-empty check before using the separator list.
  • Clarify the comment around skipping null devices and the resulting length mismatch.
deepin-devicemanager/src/Page/PageDetail.cpp
Make clearWidget safely disconnect and clear DetailButtons before delayed deletion to avoid stale clicked signals reaching slotBtnClicked.
  • Copy m_ListDetailButton into a local list, then clear the member list.
  • Disconnect each DetailButton’s clicked signal from slotBtnClicked before calling deleteLater().
  • Keep the existing deleteLater() pattern while ensuring no further clicks are delivered from buttons scheduled for destruction.
deepin-devicemanager/src/Page/PageDetail.cpp
Make slotBtnClicked robust against stale buttons and mismatched TextBrowser lists, preventing invalid index access and SEGV.
  • Replace manual foreach+break index search with m_ListDetailButton.indexOf(button).
  • Add bounds checks on the resulting index against m_ListTextBrowser.size(), returning early for missing/stale buttons.
  • Remove redundant if(button) guard before updateText() and rely on the earlier null check.
  • Preserve overall behavior for valid buttons while ignoring clicks from buttons no longer present in the list.
deepin-devicemanager/src/Page/PageDetail.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

★ 总体评分:95分

■ 【总体评价】

代码精准修复了因列表长度计算错误和延迟销毁期间信号槽未断开导致的数组越界与野指针崩溃问题
逻辑严密且注释清晰,无任何安全漏洞,因缺少对同类关联容器的一致性防御性清理扣5分

■ 【详细分析】

  • 1.语法逻辑(完全正确)✓

修复了 PageDetail::showDeviceInfo 中使用固定索引 lstInfo.size() - 1 访问 m_ListDetailSeperator 导致的越界错误,改用 last() 并增加 isEmpty() 校验,完美契合因 continue 跳过空设备导致的实际长度小于预期长度的场景。在 PageDetail::slotBtnClicked 中使用 indexOf 替代手动遍历,并增加与 m_ListTextBrowser 的边界交叉校验,彻底杜绝了过期点击引发的越界访问。
建议:在 clearWidget 中,建议对 m_ListTextBrowserm_ListDetailSeperator 容器也增加 isEmpty() 判断或采用相同的延迟安全清理模式,保持防御逻辑的一致性

  • 2.代码质量(良好)✓

代码去除了冗余的 if(button) 判空,简化了代码路径。注释非常详尽,准确解释了“为什么”要这样修改(如说明 continue 跳过空设备的影响、解释 deleteLater 期间的事件派发机制),极大提升了可维护性。
建议:clearWidget 中的 widget = nullptr; 仅修改了局部拷贝 listDetailButton 中的指针,并未修改 m_ListDetailButton(已提前 clear),该行属于无效赋值,建议删除以避免误导

  • 3.代码性能(无性能问题)✓

使用 QList::indexOf() 替代原有的 foreach 遍历查找,底层实现均为线性查找,时间复杂度稳定在 O(N),未引入额外的性能开销。增加的 isEmpty() 判断时间复杂度为 O(1),对性能无负面影响。

  • 4.代码安全(存在0个安全漏洞)✓

漏洞对比统计:新增漏洞 0 个,减少漏洞 2 个,持平 0 个
本次提交成功消除了原有的数组越界读取和悬空指针解引用引发的本地拒绝服务风险,未引入新的安全漏洞

  • 建议:继续保持这种针对异步销毁与状态不同步场景的防御性编程习惯

■ 【改进建议代码示例】

diff --git a/deepin-devicemanager/src/Page/PageDetail.cpp b/deepin-devicemanager/src/Page/PageDetail.cpp
index eb214322..improved_version 100644
--- a/deepin-devicemanager/src/Page/PageDetail.cpp
+++ b/deepin-devicemanager/src/Page/PageDetail.cpp
@@ -360,10 +360,10 @@ void PageDetail::clearWidget()
 
     QList<DetailButton *> listDetailButton = m_ListDetailButton;
     m_ListDetailButton.clear();
-    // 清空DetailButton,先断开clicked信号,避免延迟销毁期间旧按钮仍派发点击到slotBtnClicked
+    // 清空DetailButton,先断开信号避免延迟销毁期间旧按钮派发事件
     foreach (auto widget, listDetailButton) {
         disconnect(widget, &DetailButton::clicked, this, &PageDetail::slotBtnClicked);
         widget->deleteLater();
-        widget = nullptr;
     }
+

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