fix: resolve mutex deadlock in async file info query#3772
Merged
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplaces RAII-based mutex locking with explicit lock/unlock and wrapper callbacks in AsyncFileInfo and SyncFileInfo async initQuerierAsync paths to avoid deadlocks when async callbacks re-enter the same lock. Sequence diagram for updated async initQuerierAsync locking behaviorsequenceDiagram
actor Caller
participant AsyncFileInfo
participant DfmFileInfo
participant d_lock
participant UserCallback
Caller->>AsyncFileInfo: asyncQueryDfmFileInfo(ioPriority, func, userData)
AsyncFileInfo->>d_lock: lock()
AsyncFileInfo->>DfmFileInfo: initQuerierAsync(ioPriority, callback, userData)
AsyncFileInfo->>d_lock: unlock()
DfmFileInfo-->>AsyncFileInfo: callback(success, data)
AsyncFileInfo->>d_lock: unlock()
AsyncFileInfo-->>UserCallback: func(success, data)
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 3 issues, and left some high level feedback:
- The new manual lock/unlock flow in both async callbacks risks double-unlocking or unlocking an unlocked mutex if
initQuerierAsyncinvokes the callback synchronously (callback unlocks, then the outer function unlocks again), which is undefined behavior; consider limiting the lock to local state changes and not holding it across the async call instead of unlocking from within the callback. - The callbacks capture
thisand accessdafter releasing the lock without any lifetime guarantees for the owning object, which may lead to use-after-free if the object is destroyed before the async callback runs; consider using aQPointer,std::weak_ptr, or equivalent to check liveness before accessing members. - If
initQuerierAsynccan fail or throw before registering the callback, the current pattern leaves the lock in an unlocked-but-expected-to-be-locked state for future operations; it would be safer to use RAII (e.g.,QMutexLocker) for the critical section around local state and remove the cross-callback unlock responsibility altogether.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new manual lock/unlock flow in both async callbacks risks double-unlocking or unlocking an unlocked mutex if `initQuerierAsync` invokes the callback synchronously (callback unlocks, then the outer function unlocks again), which is undefined behavior; consider limiting the lock to local state changes and not holding it across the async call instead of unlocking from within the callback.
- The callbacks capture `this` and access `d` after releasing the lock without any lifetime guarantees for the owning object, which may lead to use-after-free if the object is destroyed before the async callback runs; consider using a `QPointer`, `std::weak_ptr`, or equivalent to check liveness before accessing members.
- If `initQuerierAsync` can fail or throw before registering the callback, the current pattern leaves the lock in an unlocked-but-expected-to-be-locked state for future operations; it would be safer to use RAII (e.g., `QMutexLocker`) for the critical section around local state and remove the cross-callback unlock responsibility altogether.
## Individual Comments
### Comment 1
<location path="src/dfm-base/file/local/asyncfileinfo.cpp" line_range="644-646" />
<code_context>
-
- QMutexLocker lk(&d->lock);
- d->dfmFileInfo->initQuerierAsync(ioPriority, func, userData);
+ auto callback = [this, func](bool success, void *data) {
+ d->lock.unlock();
+ func(success, data);
+ };
+ d->lock.lock();
</code_context>
<issue_to_address>
**issue (bug_risk):** Unlocking `d->lock` in both the callback and after `initQuerierAsync` risks double-unlock and UB.
The flow currently is:
1) `d->lock.lock();`
2) `d->dfmFileInfo->initQuerierAsync(ioPriority, callback, userData);`
3) `d->queringAttribute = false;`
4) `d->lock.unlock();`
Your callback also calls `d->lock.unlock();`. If `initQuerierAsync` ever invokes the callback synchronously (or before step 4), you will unlock the same mutex twice from the same thread, which is UB. Even if the callback is always asynchronous, it will still attempt to unlock a mutex that is not locked at that time, which is also UB.
Please avoid manual unlocks here. Keep the `QMutexLocker` RAII pattern, and if you need the lock released before calling `func`, use a separate callback that is invoked after leaving the locked region.
</issue_to_address>
### Comment 2
<location path="src/dfm-base/file/local/asyncfileinfo.cpp" line_range="648-645" />
<code_context>
+ d->lock.unlock();
+ func(success, data);
+ };
+ d->lock.lock();
+ d->dfmFileInfo->initQuerierAsync(ioPriority, callback, userData);
d->queringAttribute = false;
+ d->lock.unlock();
return true;
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Replacing `QMutexLocker` with manual lock/unlock increases risk of early-return paths leaking the lock.
Using `QMutexLocker` ensured the mutex was always released on every exit path. With manual `lock()`/`unlock()`, any new early return or exception here could easily leave the mutex locked and cause a deadlock. If you need to release the lock before calling `func`, you can still use `QMutexLocker` and explicitly call `locker.unlock()` at the desired point instead of managing the lock manually.
Suggested implementation:
```cpp
QMutexLocker locker(&d->lock);
auto callback = [this, func](bool success, void *data) {
QMutexLocker callbackLocker(&d->lock);
callbackLocker.unlock();
func(success, data);
};
d->dfmFileInfo->initQuerierAsync(ioPriority, callback, userData);
d->queringAttribute = false;
locker.unlock();
```
1. Ensure `QMutexLocker` is already included in this translation unit (usually via `<QMutexLocker>`). If not, add the appropriate include at the top of the file.
2. If the outer scope already uses a `QMutexLocker` for `d->lock` before this snippet, you should adapt the code to reuse that locker instead of creating a new one, to avoid double-locking.
</issue_to_address>
### Comment 3
<location path="src/dfm-base/file/local/syncfileinfo.cpp" line_range="90-91" />
<code_context>
-
- QMutexLocker lk(&d->lock);
- d->dfmFileInfo->initQuerierAsync(ioPriority, func, userData);
+ auto callback = [this, func](bool success, void *data) {
+ d->lock.unlock();
+ func(success, data);
+ };
</code_context>
<issue_to_address>
**issue (bug_risk):** The same double-unlock / unlock-without-lock issue exists here as in `AsyncFileInfo`.
The sequence here is:
1) `d->lock.lock();`
2) `d->dfmFileInfo->initQuerierAsync(ioPriority, callback, userData);`
3) `d->lock.unlock();`
and the callback also calls `d->lock.unlock()`. This can cause undefined behavior if the callback runs synchronously or after the lock was already released.
Keep using `QMutexLocker` and, if you need to release the lock before calling `func`, explicitly unlock just before invoking `func` rather than from within the async callback.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
liyigang1
force-pushed
the
master-fix
branch
3 times, most recently
from
b0ac263 to
f68f40f
Compare
Contributor
|
TAG Bot New tag: 6.5.137 |
The async initQuerierAsync methods in SyncFileInfo and AsyncFileInfo used QMutexLocker (RAII lock guard) which keeps the mutex locked until the function returns. However, the async callback may be invoked while the mutex is still held, and if the callback or its callers attempt to acquire the same lock, a deadlock occurs. Changed to manual lock management and unlock the mutex in the wrapper callback before invoking the user callback to prevent the deadlock. Log: Fixed deadlock in async file info initialization Influence: 1. Test async file info query during file operations 2. Verify no deadlock occurs under concurrent access 3. Check file info query works correctly with sync and async paths 4. Test with files on various filesystem types 5. Verify lock is properly released in all callback paths fix: 修复异步文件信息查询中的互斥锁死锁问题 SyncFileInfo 和 AsyncFileInfo 中的异步 initQuerierAsync 方法使用了 QMutexLocker（RAII锁守卫），该守卫在函数返回前一直持有互斥锁。然而， 异步回调可能在互斥锁仍被持有时就被调用，如果回调或其调用者尝试获取同一 把锁，就会发生死锁。改为手动管理锁的生命周期，并在调用用户回调函数之前 先在包装回调中释放互斥锁，以防止死锁发生。 Log: 修复异步文件信息初始化时的死锁问题 Influence: 1. 测试文件操作期间的异步文件信息查询 2. 验证并发访问下不会发生死锁 3. 检查文件信息查询在同步和异步路径下均正常工作 4. 测试各种文件系统类型 5. 验证锁在所有回调路径中被正确释放
Contributor
deepin pr auto review这是一份针对您提供的 Git Diff 的代码审查报告。本次修改主要涉及对异步/同步文件信息查询接口的空指针防护以及回调生命周期/锁作用域的优化。 总体而言，这次修改修复了一个非常关键的多线程并发与对象生命周期引发的崩溃问题，并增加了必要的入参校验。以下是详细的审查意见： 1. 语法与逻辑文件： - if (d->queringAttribute)
+ if (d->queringAttribute || !func)
return false;
文件： - QMutexLocker locker(&d->lock);
- if (d->dfmFileInfo)
- d->dfmFileInfo->initQuerierAsync(ioPriority, func, userData);
+ if (!func)
+ return;
+
+ auto lockGuard = std::make_shared<std::unique_lock<QMutex>>(d->lock);
+ if (d->dfmFileInfo) {
+ auto wrapper = [lockGuard, func](bool success, void *data) {
+ lockGuard->unlock();
+ func(success, data);
+ };
+ d->dfmFileInfo->initQuerierAsync(ioPriority, wrapper, userData);
+ }
2. 代码质量
3. 代码性能
4. 代码安全这里有几个潜在的安全隐患和需要确认的细节：
改进建议代码针对上述安全分析，建议对 void SyncFileInfo::initQuerierAsync(int ioPriority, FileInfo::initQuerierAsyncCallback func, void *userData)
{
if (!func)
return;
auto lockGuard = std::make_shared<std::unique_lock<QMutex>>(d->lock);
if (d->dfmFileInfo) {
// 捕获 lockGuard，延长生命周期至回调结束
auto wrapper = [lockGuard, func](bool success, void *data) {
// 先执行回调，确保回调执行期间锁仍然持有（如果业务允许）
// lockGuard 会在 lambda 析构时自动释放锁，符合 RAII 规范
func(success, data);
};
d->dfmFileInfo->initQuerierAsync(ioPriority, wrapper, userData);
}
// 如果 d->dfmFileInfo 为空，lockGuard 在此自然析构释放锁，无死锁风险
}注意：如果业务上严格要求“在调用 |
Johnson-zs
approved these changes
May 18, 2026
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Johnson-zs, liyigang1 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 |
Contributor
|
/forcemerge |
Contributor
|
This pr force merged! (status: behind) |
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.
The async initQuerierAsync methods in SyncFileInfo and AsyncFileInfo used QMutexLocker (RAII lock guard) which keeps the mutex locked until the function returns. However, the async callback may be invoked while the mutex is still held, and if the callback or its callers attempt to acquire the same lock, a deadlock occurs. Changed to manual lock management and unlock the mutex in the wrapper callback before invoking the user callback to prevent the deadlock.
Log: Fixed deadlock in async file info initialization
Influence:
fix: 修复异步文件信息查询中的互斥锁死锁问题
SyncFileInfo 和 AsyncFileInfo 中的异步 initQuerierAsync 方法使用了 QMutexLocker（RAII锁守卫），该守卫在函数返回前一直持有互斥锁。然而，
异步回调可能在互斥锁仍被持有时就被调用，如果回调或其调用者尝试获取同一
把锁，就会发生死锁。改为手动管理锁的生命周期，并在调用用户回调函数之前
先在包装回调中释放互斥锁，以防止死锁发生。
Log: 修复异步文件信息初始化时的死锁问题
Influence:
Summary by Sourcery
Bug Fixes: