Skip to content

fix(dfm-search): eliminate cross-thread race on m_strategy causing heap corruption (cherry-pick to master) - #376

Closed
pppanghu77 wants to merge 0 commit into
linuxdeepin:masterfrom
pppanghu77:bugfix/372535-mstrategy-race-master
Closed

fix(dfm-search): eliminate cross-thread race on m_strategy causing heap corruption (cherry-pick to master)#376
pppanghu77 wants to merge 0 commit into
linuxdeepin:masterfrom
pppanghu77:bugfix/372535-mstrategy-race-master

Conversation

@pppanghu77

@pppanghu77 pppanghu77 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Cherry-pick of PR #374 (commit 290eec5) to master.

修复内容

修复 BUG-372535:搜索压测过程中产生文件管理器 coredump。

根因:util-dfm 的可取消搜索设计中,SearchWorker::cancelSearch()Qt::DirectConnection 在调用线程读取 worker 的 std::unique_ptr<BaseSearchStrategy> m_strategy,与 doSearch()(引擎工作线程)对该指针的创建/重置形成跨线程无锁竞争,导致堆损坏 → Lucene++ 分配内存时 SIGBUS。

改动

  • 移除 cancelSearch() 槽与 requestCancel 信号,主线程不再经 DirectConnection 读 worker 的 m_strategy
  • 通过 setCancelledFlag() 向策略注入引擎级 atomic 取消标志,策略改读 m_cancelledRef
  • 删除冗余的 BaseSearchStrategy::m_cancelled,改用运行时检查(非 Q_ASSERT,Release 构建会被移除)
  • m_engineCancelled 为空,doSearch 报错并中止
  • 所有策略的 m_cancelledRef 解引用处增加空指针防御
  • SearchCancellationGuard 构造前用局部 dummy 兜底,避免空指针解引用
  • doSyncSearch() 开头重置 m_cancelled,超时置位,修复同步搜索取消

PMS: https://pms.uniontech.com/bug-view-372535.html
Original PR (V20 develop/meagle-20260526): #374
Cherry-pick of commit 290eec5, clean (no conflicts).

Summary by Sourcery

Refactor the search cancellation mechanism to use an engine-level atomic flag instead of cross-thread access to strategy state, preventing races and heap corruption during dfm search.

Bug Fixes:

  • Fix a cross-thread race on search strategy cancellation that could corrupt heap memory and cause coredumps under load.
  • Ensure synchronous and asynchronous searches correctly respect cancellation and timeout conditions.

Enhancements:

  • Inject a shared engine-level atomic cancellation flag into search strategies and worker to provide safe, low-latency cancellation signaling.
  • Harden search strategies and guards with null-pointer checks and warnings when cancellation flags are not properly initialized.

@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 @pppanghu77, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@sourcery-ai

sourcery-ai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Reviewer's Guide

Refactors the search cancellation mechanism to use a shared engine-level atomic flag instead of cross-thread access to the worker’s strategy pointer, eliminating a race that caused heap corruption and tightening cancellation handling across synchronous and asynchronous searches.

Sequence diagram for updated search cancellation flow

sequenceDiagram
    actor Client
    participant GenericSearchEngine
    participant SearchWorker
    participant BaseSearchStrategy

    Client->>GenericSearchEngine: init()
    GenericSearchEngine->>SearchWorker: setEngineCancelledFlag(&m_cancelled)

    Client->>GenericSearchEngine: search(query, options, searchType)
    GenericSearchEngine->>SearchWorker: doSearch(query, options, searchType)
    SearchWorker->>BaseSearchStrategy: search(query)
    SearchWorker->>BaseSearchStrategy: setCancelledFlag(m_engineCancelled)

    Client->>GenericSearchEngine: cancel()
    GenericSearchEngine->>GenericSearchEngine: m_cancelled.store(true)
    loop [search in worker thread]
        BaseSearchStrategy->>BaseSearchStrategy: [check m_cancelledRef->load()]
    end
Loading

File-Level Changes

Change Details Files
Introduce engine-level atomic cancellation flag injection into strategies and workers, replacing per-strategy flags.
  • Add BaseSearchStrategy::setCancelledFlag(std::atomic) to inject an engine-owned atomic flag used for cancellation checks.
  • Replace BaseSearchStrategy’s std::atomic member with a std::atomic and initialize it to nullptr.
  • Warn and ignore when a null cancellation flag is injected instead of asserting, ensuring release builds preserve diagnostics.
src/dfm-search/dfm-search-lib/core/searchstrategy/basesearchstrategy.h
Move cancellation control from a DirectConnection-based signal/slot to a shared atomic flag managed by GenericSearchEngine and read by SearchWorker and strategies.
  • Remove GenericSearchEngine::requestCancel signal and the connection to SearchWorker::cancelSearch using Qt::DirectConnection.
  • In GenericSearchEngine::init, inject &m_cancelled into SearchWorker via setEngineCancelledFlag.
  • Implement engine-level cancellation in GenericSearchEngine::cancel() and search timeout handling by only manipulating m_cancelled and stopping timers.
src/dfm-search/dfm-search-lib/core/genericsearchengine.cpp
src/dfm-search/dfm-search-lib/core/genericsearchengine.h
Ensure SearchWorker uses the injected engine cancellation flag and no longer exposes a cancelSearch slot, preventing cross-thread races on m_strategy.
  • Add std::atomic* m_engineCancelled to SearchWorker and a setter setEngineCancelledFlag(std::atomic*).
  • In SearchWorker::doSearch, validate m_engineCancelled is non-null, abort with error if null, and inject it into the strategy via setCancelledFlag.
  • Remove the cancelSearch() slot to avoid accessing m_strategy from other threads.
src/dfm-search/dfm-search-lib/core/searchstrategy/searchworker.cpp
src/dfm-search/dfm-search-lib/core/searchstrategy/searchworker.h
Update all concrete search strategies to use the injected cancellation flag pointer with null checks instead of owning their own atomic flag.
  • Remove local m_cancelled.reset() calls from search() in filename, content, OCR text, and realtime strategies; rely on engine-level flag.
  • Guard cancellation checks with m_cancelledRef non-null checks in loops processing search results or filesystem traversal.
  • Update SearchCancellationGuard usage to accept m_cancelledRef and ensure it is non-null via an internal dummy fallback to avoid null dereference.
  • Pass m_cancelledRef to CancellableCollector instead of &m_cancelled in all Lucene-based strategies.
  • Change cancel() implementations in concrete strategies to store(true) into m_cancelledRef only when non-null.
src/dfm-search/dfm-search-lib/contentsearch/contentstrategies/indexedstrategy.cpp
src/dfm-search/dfm-search-lib/ocrtextsearch/ocrtextstrategies/indexedstrategy.cpp
src/dfm-search/dfm-search-lib/filenamesearch/filenamestrategies/indexedstrategy.cpp
src/dfm-search/dfm-search-lib/filenamesearch/filenamestrategies/realtimestrategy.cpp
Fix synchronous search cancellation semantics to align with the new engine-level flag and avoid stale cancellation state.
  • Reset m_cancelled to false at the start of GenericSearchEngine::doSyncSearch to clear any previous cancellation.
  • On sync search timeout, set m_cancelled to true instead of emitting requestCancel, so the worker observes the cancellation through the shared flag.
src/dfm-search/dfm-search-lib/core/genericsearchengine.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.语法逻辑(完全正确)✓

重构精准解决了旧设计中策略在search()入口重置m_cancelled导致的竞态窗口,以及doSyncSearch未重置引擎级标志导致的状态残留。GenericSearchEngine::init()在启动线程前注入指针,doSyncSearch()补齐了m_cancelled.store(false)重置,SearchWorker::doSearch()对m_engineCancelled进行空指针拦截并中断执行,确保后续SearchCancellationGuard和CancellableCollector接收到的必定是非空指针,逻辑闭环完美。
建议:无

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

注释详尽解释了不使用Q_ASSERT的原因及注入时机的必要性,命名m_cancelledRef清晰表达了指针语义。不足之处在于ContentIndexedStrategy、FileNameIndexedStrategy、FileNameRealTimeStrategy、OcrTextIndexedStrategy四个子类中,m_cancelledRef && m_cancelledRef->load()的判空与读取逻辑重复出现了近十次。
潜在问题:重复的判空读取代码增加了维护成本,若未来修改取消标志判断逻辑需同步修改多处。
建议:在BaseSearchStrategy基类中增加一个inline函数如bool isCancelled() const { return m_cancelledRef && m_cancelledRef->load(); },供所有子类统一调用。

  • 3.代码性能(高效)✓

移除了Qt::DirectConnection跨线程信号槽调用的固定开销,改为直接的原子变量内存读写。虽然将m_cancelled.load()改为m_cancelledRef && m_cancelledRef->load()增加了一次指针判空,但由于m_cancelledRef在单次搜索生命周期内不可变且处于热循环中,编译器极易将其优化为分支预测友好的代码,实际性能损耗可忽略不计。
建议:无

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

漏洞对比统计:新增漏洞 0 个,减少漏洞 0 个,持平 0 个
代码通过std::atomic保证了多线程读写的内存序安全,彻底消除了原有的竞态条件漏洞。所有外部传入的指针均在入口处做了严格的空指针校验,未发现命令注入、缓冲区溢出等安全漏洞。
建议:无

■ 【改进建议代码示例】

// basesearchstrategy.h
class BaseSearchStrategy : public QObject
{
    // ... 其他代码保持不变 ...

protected:
    /**
     * @brief 检查当前是否已被取消
     * @return 若标志已注入且为true则返回true,否则返回false
     */
    inline bool isCancelled() const
    {
        return m_cancelledRef && m_cancelledRef->load(std::memory_order_relaxed);
    }

    SearchOptions m_options;
    SearchResultList m_results;
    std::atomic<bool> *m_cancelledRef { nullptr };
};

// 各策略中简化调用(以 ContentIndexedStrategy 为例)
void ContentIndexedStrategy::processSearchResults(const Lucene::IndexSearcherPtr &searcher, ...)
{
    // ...
    for (int32_t i = 0; i < docsSize; ++i) {
        if (isCancelled()) { // 旧:if (m_cancelledRef && m_cancelledRef->load())
            qInfo() << "Content search cancelled";
            break;
        }
        // ...
    }
}

void FileNameRealTimeStrategy::search(const SearchQuery &query)
{
    // ...
    while (!directoryStack.isEmpty() && count < maxResults && !isCancelled()) {
        // ...
        for (const QFileInfo &info : std::as_const(entries)) {
            if (isCancelled() || count >= maxResults) {
                break;
            }
            // ...
        }
    }
}

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pppanghu77

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

@pppanghu77 pppanghu77 closed this Aug 5, 2026
@pppanghu77
pppanghu77 force-pushed the bugfix/372535-mstrategy-race-master branch from 2c6d10b to 0b86533 Compare August 5, 2026 03:00
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