fix(dfm-search): eliminate cross-thread race on m_strategy causing heap corruption - #375
Merged
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Johnson-zs, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Johnson-zs 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 |
Reviewer's GuideRefactors dfm-search cancellation to use an engine-level atomic flag injected into strategies, removing cross-thread access to SearchWorker::m_strategy and hardening cancellation handling (including sync searches) with null-guarded pointers and timeout handling. Sequence diagram for engine-level cancellation flag in dfm-searchsequenceDiagram
actor User
participant GenericSearchEngine
participant SearchWorker
participant BaseSearchStrategy
User->>GenericSearchEngine: search(query)
GenericSearchEngine->>SearchWorker: doSearch(query, options, searchType)
Note over GenericSearchEngine,SearchWorker: init() already called
GenericSearchEngine->>SearchWorker: setEngineCancelledFlag(&m_cancelled)
SearchWorker->>BaseSearchStrategy: setCancelledFlag(m_engineCancelled)
SearchWorker->>BaseSearchStrategy: search(query)
User->>GenericSearchEngine: cancel()
GenericSearchEngine->>GenericSearchEngine: m_cancelled.store(true)
loop search loop
BaseSearchStrategy->>BaseSearchStrategy: m_cancelledRef->load()
alt [cancel flag is true]
BaseSearchStrategy-->>SearchWorker: searchFinished(results)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…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
deepin pr auto review★ 总体评分：95分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 diff --git a/src/dfm-search/dfm-search-lib/core/searchstrategy/basesearchstrategy.h b/src/dfm-search/dfm-search-lib/core/searchstrategy/basesearchstrategy.h
index 205e36fd..8a2c1f91 100644
--- a/src/dfm-search/dfm-search-lib/core/searchstrategy/basesearchstrategy.h
+++ b/src/dfm-search/dfm-search-lib/core/searchstrategy/basesearchstrategy.h
@@ -89,6 +89,12 @@ protected:
SearchResultList m_results;
std::atomic<bool> *m_cancelledRef { nullptr };
+
+ /**
+ * @brief 安全读取取消标志，封装空指针校验逻辑供派生类统一调用
+ */
+ inline bool isCancelled() const {
+ return m_cancelledRef && m_cancelledRef->load();
+ }
};
DFM_SEARCH_END_NS
diff --git a/src/dfm-search/dfm-search-lib/recentsearch/recentstrategies/recentsearchstrategy.cpp b/src/dfm-search/dfm-search-lib/recentsearch/recentstrategies/recentsearchstrategy.cpp
index 8aa452bc..d4f2c1e3 100644
--- a/src/dfm-search/dfm-search-lib/recentsearch/recentstrategies/recentsearchstrategy.cpp
+++ b/src/dfm-search/dfm-search-lib/recentsearch/recentstrategies/recentsearchstrategy.cpp
@@ -202,7 +202,7 @@ void RecentSearchStrategy::search(const SearchQuery &query)
// Step 1: 从 DBus 拉取全部最近使用记录
QList<RecentItem> items = fetchRecentItems();
- if (m_cancelledRef && m_cancelledRef->load()) {
+ if (isCancelled()) {
emit searchFinished(m_results);
return;
}
@@ -236,7 +236,7 @@ void RecentSearchStrategy::search(const SearchQuery &query)
int count = 0;
for (const RecentItem &item : std::as_const(items)) {
- if ((m_cancelledRef && m_cancelledRef->load()) || count >= maxResults) {
+ if (isCancelled() || count >= maxResults) {
break;
}
|
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: blocked) |
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.
Log: 消除主线程与 worker 线程对 SearchWorker::m_strategy 的无锁竞争（SIGBUS 堆损坏根因），补齐同步搜索取消标志重置，增加 m_cancelledRef 空指针防御
Bug: https://pms.uniontech.com/bug-view-372535.html
Summary by Sourcery
Unify search cancellation around a shared engine-level atomic flag to remove unsafe cross-thread access to strategies and improve cancellation reliability for both async and sync searches.
Bug Fixes:
Enhancements: