fix(dfm-search): eliminate cross-thread race on m_strategy causing heap corruption - #374
Merged
Johnson-zs merged 1 commit intoAug 4, 2026
Conversation
Reviewer's GuideRefactors dfm-search cancellation to remove a cross-thread race on SearchWorker::m_strategy by eliminating the direct cancel signal/slot and introducing a shared engine-level atomic cancellation flag that is injected into all search strategies. Sequence diagram for updated dfm-search cancellation flowsequenceDiagram
actor MainThread
participant GenericSearchEngine
participant SearchWorker
participant BaseSearchStrategy
MainThread->>GenericSearchEngine: init()
GenericSearchEngine->>SearchWorker: setEngineCancelledFlag(&m_cancelled)
MainThread->>GenericSearchEngine: search(query, options, searchType)
GenericSearchEngine->>SearchWorker: doSearch(query, options, searchType)
SearchWorker->>BaseSearchStrategy: setCancelledFlag(m_engineCancelled)
SearchWorker->>BaseSearchStrategy: search(query)
MainThread->>GenericSearchEngine: cancel()
GenericSearchEngine->>GenericSearchEngine: m_cancelled.store(true)
BaseSearchStrategy-->>BaseSearchStrategy: [m_cancelledRef->load()]
MainThread->>GenericSearchEngine: searchSync(query)
GenericSearchEngine->>GenericSearchEngine: m_cancelled.store(false)
GenericSearchEngine-->>GenericSearchEngine: [timeout]
GenericSearchEngine->>GenericSearchEngine: m_cancelled.store(true)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Dereferencing
m_cancelledRefin the various strategies and inSearchCancellationGuardassumes it is always non-null; relying only onQ_ASSERTmeans release builds will still have UB if the flag is not injected, so consider making this a reference or otherwise guaranteeing a non-null default (e.g., a fallback static flag) to enforce safety in all builds. - Given that
SearchWorker::doSearchinjects the engine cancel flag only whenm_engineCancelledis non-null, it may be safer to assert or early-fail indoSearchwhenm_engineCancelledis missing, to avoid silently running strategies without a valid cancellation flag and then dereferencingm_cancelledRef.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Dereferencing `m_cancelledRef` in the various strategies and in `SearchCancellationGuard` assumes it is always non-null; relying only on `Q_ASSERT` means release builds will still have UB if the flag is not injected, so consider making this a reference or otherwise guaranteeing a non-null default (e.g., a fallback static flag) to enforce safety in all builds.
- Given that `SearchWorker::doSearch` injects the engine cancel flag only when `m_engineCancelled` is non-null, it may be safer to assert or early-fail in `doSearch` when `m_engineCancelled` is missing, to avoid silently running strategies without a valid cancellation flag and then dereferencing `m_cancelledRef`.
## Individual Comments
### Comment 1
<location path="src/dfm-search/dfm-search-lib/core/searchstrategy/basesearchstrategy.h" line_range="64-72" />
<code_context>
+ * 设 true，工作线程即时响应。flag 必须非空，由 SearchWorker::doSearch
+ * 创建策略后立即注入；未注入视为编程错误（Q_ASSERT 暴露）。
+ */
+ void setCancelledFlag(std::atomic<bool> *flag)
+ {
+ Q_ASSERT(flag);
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid relying on a potentially-null cancellation flag pointer at runtime
`BaseSearchStrategy` holds a `std::atomic<bool>*` that is default-initialized to `nullptr`, and the only protection is a `Q_ASSERT` in `setCancelledFlag`. If the strategy is used without `SearchWorker::setEngineCancelledFlag` (or if `setCancelledFlag` is forgotten), any use of `m_cancelledRef` in `cancel()`/search will dereference `nullptr` in release builds. Please either (1) provide an internal owned `atomic<bool>` fallback when no external flag is injected, (2) assert `m_cancelledRef` at every dereference, or (3) require the flag via constructor so `nullptr` is impossible by design.
</issue_to_address>
### Comment 2
<location path="src/dfm-search/dfm-search-lib/core/searchstrategy/searchworker.h" line_range="32-36" />
<code_context>
void setStrategyFactory(std::unique_ptr<SearchStrategyFactory> factory);
+ void setEngineCancelledFlag(std::atomic<bool> *flag) { m_engineCancelled = flag; }
+
public Q_SLOTS:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Make engine cancellation flag injection and usage stricter to prevent null propagation
`SearchWorker::doSearch` only calls `m_strategy->setCancelledFlag` when `m_engineCancelled` is non-null, and `m_engineCancelled` defaults to `nullptr`. If `setEngineCancelledFlag` is never called, both `m_engineCancelled` and the strategy’s `m_cancelledRef` remain unset, yet strategies assume a valid pointer and dereference it. To make this contract safer, consider enforcing that the flag is always set (e.g. add a `Q_ASSERT(m_engineCancelled)` before strategy creation, assert in `setEngineCancelledFlag` on null, or pass the flag into `SearchWorker`’s constructor so it can never be `nullptr`). This helps avoid latent null-deref crashes when the injection step is missed.
Suggested implementation:
```c
void setStrategyFactory(std::unique_ptr<SearchStrategyFactory> factory);
/**
* Injects the engine cancellation flag.
* This pointer must not be null; strategies assume a valid flag and may dereference it.
*/
void setEngineCancelledFlag(std::atomic<bool> *flag)
{
Q_ASSERT(flag);
m_engineCancelled = flag;
}
public Q_SLOTS:
```
` section.
Here are the code changes:
<file_operations>
<file_operation operation="edit" file_path="src/dfm-search/dfm-search-lib/core/searchstrategy/searchworker.h">
<<<<<<< SEARCH
void setStrategyFactory(std::unique_ptr<SearchStrategyFactory> factory);
void setEngineCancelledFlag(std::atomic<bool> *flag) { m_engineCancelled = flag; }
public Q_SLOTS:
=======
void setStrategyFactory(std::unique_ptr<SearchStrategyFactory> factory);
/**
* Injects the engine cancellation flag.
* This pointer must not be null; strategies assume a valid flag and may dereference it.
*/
void setEngineCancelledFlag(std::atomic<bool> *flag)
{
Q_ASSERT(flag);
m_engineCancelled = flag;
}
public Q_SLOTS:
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
1. In `searchworker.cpp` (or wherever strategies are created/configured), add a `Q_ASSERT(m_engineCancelled);` before constructing/initializing `m_strategy` and before calling `m_strategy->setCancelledFlag(...)`. This ensures that if the injection step is missed, the failure is caught early in debug builds.
2. Verify that any existing call sites of `setEngineCancelledFlag` never pass `nullptr`; if there are conditional calls, they should be refactored so the flag is always created and passed, or the condition is removed.
3. Consider updating any class invariants or documentation (e.g. comments in the class header) to explicitly state that `SearchWorker` requires a non-null cancellation flag before starting a search.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
/retest |
Contributor
Author
|
/retest-required |
Contributor
Author
|
/retest |
pppanghu77
force-pushed
the
bugfix/372535-mstrategy-race
branch
from
August 4, 2026 09:06
2ef681e to
214bb10
Compare
Johnson-zs
approved these changes
Aug 4, 2026
…ap corruption - Remove cancelSearch() slot and requestCancel signal so the main thread no longer reads the worker's m_strategy via Qt::DirectConnection (root cause of the SIGBUS heap corruption) - Inject the engine-level atomic cancellation flag into strategies via setCancelledFlag(); strategies now read m_cancelledRef instead of a per-strategy flag - Drop the redundant BaseSearchStrategy::m_cancelled and enforce mandatory injection with runtime check (not Q_ASSERT, which is stripped in Release) - If m_engineCancelled is null in doSearch, emit error and abort instead of proceeding with null m_cancelledRef - Add null guards on all m_cancelledRef dereferences in strategies (cancel() and search loops) - Reset m_cancelled at the start of doSyncSearch() to match async search(), fixing silent empty results after a cancelled sync search - Set m_cancelled on sync search timeout so the worker actually stops Log: 消除主线程与 worker 线程对 SearchWorker::m_strategy 的无锁竞争（SIGBUS 堆损坏根因），补齐同步搜索取消标志重置，增加 m_cancelledRef 空指针防御 Bug: https://pms.uniontech.com/bug-view-372535.html
pppanghu77
force-pushed
the
bugfix/372535-mstrategy-race
branch
from
August 4, 2026 12:25
214bb10 to
290eec5
Compare
deepin pr auto review★ 总体评分：90分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 // 文件：contentsearch/contentstrategies/indexedstrategy.cpp
// 修复 CancellableCollector 传参不一致问题
void ContentIndexedStrategy::performContentSearch(const SearchQuery &query)
{
// RAII 守护类：自动管理取消标志的生命周期
// 若 m_cancelledRef 为空（非正常调用链），使用局部 dummy 标志避免空指针解引用
std::atomic<bool> dummyCancelled { false };
std::atomic<bool> *cancelledFlag = m_cancelledRef ? m_cancelledRef : &dummyCancelled;
SearchCancellationGuard guard(cancelledFlag);
try {
// 获取索引目录
// ...
Collection<ScoreDocPtr> scoreDocs;
try {
// 修复：统一使用经过空指针保护的 cancelledFlag，而不是直接使用 m_cancelledRef
boost::shared_ptr<CancellableCollector> collector = newLucene<CancellableCollector>(cancelledFlag, maxResults);
// 执行搜索，使用自定义收集器
qInfo() << "Content search execution start:" << query.keyword();
// ...// 文件：core/searchstrategy/searchworker.h
// 补充 setEngineCancelledFlag 的防御性检查，保持架构风格一致
void setEngineCancelledFlag(std::atomic<bool> *flag)
{
if (!flag) {
qWarning("SearchWorker::setEngineCancelledFlag: flag is null, search will abort on doSearch");
}
m_engineCancelled = flag;
} |
Contributor
Author
|
/retest-required |
Johnson-zs
approved these changes
Aug 4, 2026
Contributor
|
/merge |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Johnson-zs, pppanghu77 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 |
|
This pr cannot be merged! (status: unstable) |
Contributor
Author
|
/forcemegre |
Johnson-zs
merged commit Aug 4, 2026
83a8361
into
linuxdeepin:develop/meagle-20260526
17 of 20 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
修复内容
修复 BUG-372535：搜索压测过程中产生文件管理器 coredump。
根因：util-dfm 的可取消搜索设计中，
SearchWorker::cancelSearch()经Qt::DirectConnection在调用线程读取 worker 的std::unique_ptr<BaseSearchStrategy> m_strategy，与doSearch()（引擎工作线程）对该指针的创建/重置形成跨线程无锁竞争，导致堆损坏 → Lucene++ 分配内存时 SIGBUS。改动
cancelSearch()槽与requestCancel信号，主线程不再经DirectConnection读 worker 的m_strategysetCancelledFlag()向策略注入引擎级 atomic 取消标志，策略改读m_cancelledRefBaseSearchStrategy::m_cancelled，用Q_ASSERT强制注入doSyncSearch()开头重置m_cancelled，超时置位，使 worker 真正停止m_cancelledRefPMS: https://pms.uniontech.com/bug-view-372535.html
Summary by Sourcery
Refactor the search cancellation mechanism to use an engine-level atomic flag shared with strategies, eliminating cross-thread access to the worker’s strategy pointer and preventing heap corruption during search.
Bug Fixes:
Enhancements: