fix: return QList by ref in getappItem - #3445
MyLeeJiEun wants to merge 2 commits into
Conversation
为 dde-control-center 补充 12 个核心单元的 GTest 单元测试,覆盖 DCCLocale、MetaData、KeyboardModel、DockPluginSortProxyModel、CategoryModel 等模块。新增 12 个测试文件并修改 tests/CMakeLists.txt,共 13 个文件、334 个用例(331 passed / 0 failed / 3 skipped)。 覆盖率:行 98.6% / 函数 98.9% / 分支 66.6%,均达交付达标线(函数 >80% / 行 >80% / 分支尽可能高)。 关联 issue: DDE-104
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MyLeeJiEun 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 |
|
Hi @MyLeeJiEun. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's GuideFixes CategoryModel::getAppById by avoiding pointers and iterators into temporary QList values, then updates callers and tests to use value semantics; the stacked PR also introduces the shared unit-test target, extensive component coverage, and an optional gcovr coverage report. Sequence diagram for safe app lookup and default-app operationssequenceDiagram
participant Caller
participant CategoryModel
participant Category
participant App
Caller->>CategoryModel: removeApp(id) or setDefaultApp(id)
CategoryModel->>Category: getappItem()
Category-->>CategoryModel: QList<App> by value
CategoryModel->>CategoryModel: getAppById(id)
CategoryModel->>App: isValid(app)
alt app found
CategoryModel-->>Caller: requestDelUserApp(category, app) or requestSetDefaultApp(category, app)
else app not found
CategoryModel-->>Caller: no-op
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
1. getappItem() returned QList<App> by value, creating temporaries 2. getAppById iterated three different temporaries via begin/end/cend 3. The returned const App* pointed into a destroyed temporary (UB) 4. Return const QList<App>& referencing member m_applist to fix it Influence: 1. Verify setting/removing default apps works in default-app plugin 2. Verify setDefaultApp/removeApp with a non-existent id is a no-op 3. Run unit-test binary: all 66 cases must pass fix: getappItem 改为引用返回以消除悬垂指针 1. getappItem() 原按值返回 QList<App>,产生临时对象 2. getAppById 通过 begin/end/cend 迭代了三个不同临时容器 3. 返回的 const App* 指向已销毁的临时对象,属未定义行为 4. 改为返回 const QList<App>& 引用成员 m_applist 以修复 Influence: 1. 验证默认应用插件中设置/移除默认应用功能正常 2. 验证 setDefaultApp/removeApp 传入不存在的 id 时为空操作 3. 运行单元测试:全部 66 个用例须通过
114f855 to
b973e66
Compare
|
TAG Bot New tag: 6.1.106 |
|
TAG Bot New tag: 6.1.107 |
Root Cause Analysis
CategoryModel::getAppByIdreturnedconst App*pointing into the return value ofm_category->getappItem(), which returnsQList<App>by value. The temporaryQListis destroyed at the end of the full expression, leaving a dangling pointer. CallersremoveAppandsetDefaultAppdereference this dangling pointer viaisValid(*app)and the subsequentQ_EMIT, causing undefined behavior. Additionally,std::find_ifevaluatedm_category->getappItem()three times (begin / end / cend), producing iterators into three different temporary containers — also UB.Key evidence:
categorymodel.cpp:163—getAppByIdcalledgetappItem()3× across onefind_if+ onecend()comparison; the returned&(*res)pointed into a destroyed temporary.Fix (adjusted — single-line change)
Changed
getappItem()to returnconst QList<App>&(a reference to the private memberm_applist) instead ofconst QList<App>by value. This is a single-line change incategory.h:43. With a stable reference,getAppById's three calls togetappItem()(begin / end / cend) now all refer to the samem_applist, so iterator comparisons are valid and&(*res)points into a live member — the dangling-pointer UB is eliminated.getAppById,removeApp,setDefaultApp, and the three unit tests remain at their original implementation (reverted from the earlier value-return draft).Why reference-return over the earlier value-return draft
Returning
const QList<App>&from an inline accessor that returns a member is safe — the memberm_applistoutlives every call, so there is no dangling reference. It is the minimal, least-invasive fix: one line touched, all existing call sites and signatures preserved, no ABI impact (the function is inline;Categoryis an internal plugin class with no export macro). The earlier draft changedgetAppById's signature (const App*→App) and adapted three tests; that has been reverted in favor of the smaller surface area.Change Safety Assessment
Code Safety
getappItem()returns a const reference to the private memberm_applist; the function is inline and the member outlives the call. No dangling reference.getAppById(private, two callersremoveApp/setDefaultApp) now iterates one stable container;&(*res)points into a live member element. The cross-container iterator comparison + pointer-into-destroyed-temporary UB is eliminated.getAppByIdstill returnsconst App*;removeApp/setDefaultAppunchanged. Reference-binding callers (constructor range-for,onAddApp,getAppById) do not modifym_applistduring use; copy-type callers (defappworker.cpp) obtain independent snapshots.Business Impact Scope
Affects the default-app plugin (
plugin-defaultapp). Eliminates undefined behavior when a user removes a custom app or sets a default app. No user-visible behavior change on the normal path — the fix only prevents latent crashes / memory corruption that could occur when the dangling pointer was dereferenced.Verification
Verification Suggestion
In the Default Applications settings module: verify that setting a default app and removing a custom app both work correctly; verify that a non-existent app ID is a no-op.
根因分析
CategoryModel::getAppById返回const App*,指向m_category->getappItem()的返回值,而getappItem()按值返回QList<App>。该临时QList在完整表达式结束时被销毁,留下悬垂指针。调用方removeApp和setDefaultApp通过isValid(*app)及随后的Q_EMIT解引用该悬垂指针,导致未定义行为。此外,std::find_if对m_category->getappItem()求值了三次(begin / end / cend),产生指向三个不同临时容器的迭代器——同样属于 UB。关键证据:
categorymodel.cpp:163——getAppById在一次find_if+ 一次cend()比较中调用了getappItem()共 3 次;返回的&(*res)指向已销毁的临时对象。修复方案(已调整 — 单行改动)
将
getappItem()的返回类型由按值返回const QList<App>改为返回const QList<App>&(对私有成员m_applist的引用),仅改动category.h:43一行。返回稳定引用后,getAppById对getappItem()的三次调用(begin/end/cend)均指向同一m_applist,迭代器比较合法,&(*res)指向存活成员元素,悬垂指针 UB 彻底消除。getAppById、removeApp、setDefaultApp及三个单元测试均保持原始实现(已从早期值返回草案回退)。为何选择引用返回而非先前的值返回草案
内联访问器返回成员的
const QList<App>&是安全的——成员m_applist生命周期长于任何调用,无悬垂引用。此为最小侵入修复:仅改一行,保留全部调用点与签名,无 ABI 影响(函数为内联,Category为无导出宏的插件内部类)。早期草案曾修改getAppById签名(const App*→App)并适配三个测试,已回退以缩小改动面。改动安全评估
代码安全评估
getappItem()返回对私有成员m_applist的 const 引用;函数为内联,成员生命周期长于任何调用,无悬垂引用。getAppById(私有,两个调用方removeApp/setDefaultApp)现迭代同一稳定容器;&(*res)指向存活成员元素。跨容器迭代器比较 + 指向已销毁临时的 UB 已消除。getAppById仍返回const App*;removeApp/setDefaultApp不变。引用绑定型调用方(构造函数 range-for、onAddApp、getAppById)在使用期内不修改m_applist;拷贝型调用方(defappworker.cpp)获得独立快照。业务影响范围
影响默认应用插件(
plugin-defaultapp)。消除用户移除自定义应用或设置默认应用时的未定义行为。正常路径下无用户可感知的行为变化——修复仅防止悬垂指针被解引用时可能导致的潜在崩溃 / 内存损坏。验证
验证建议
在默认应用设置模块中:验证设置默认应用和移除自定义应用功能正常;验证传入不存在的应用 ID 时为空操作。
Multica Issue: DDE-158 (id:
6a561c87-65c5-42c4-b918-d950d5798691)