fix(dfileinfo): fix heap corruption caused by unlocked gfileinfo lifecycle - #388
Conversation
…cycle - Replace direct unref-and-assign in queryInfoSync() and both async query callbacks with atomic CAS exchange plus delayed unref of the old GFileInfo, closing the double-unref / use-after-free window against concurrent readers - Also fixes the GFileInfo leak in queryInfoAsyncCallback/2 where the old pointer was overwritten without unref - Take a temporary reference on gfileinfo via g_atomic_pointer_get + g_object_ref (g_autoptr) at the entry of attributesBySelf / exists / attribute / hasAttribute / customAttribute, so readers survive concurrent refresh - The 3s low-priority delayed unref only covers the load-to-ref instruction gap; it requires the host to iterate the default GLib main context (Qt GUI hosts do) and briefly retains replaced GFileInfo on bulk refresh 修复(dfileinfo): 修复 gfileinfo 无锁生命周期导致的堆损坏 - queryInfoSync() 及两个异步查询回调中的直接 unref 加赋值改为原子 CAS 交换加旧值延迟 unref，关闭与并发读线程之间的 double-unref / use-after-free 窗口 - 同时修复 queryInfoAsyncCallback/2 直接赋值覆盖旧指针导致的 GFileInfo 泄漏 - attributesBySelf / exists / attribute / hasAttribute / customAttribute 入口通过 g_atomic_pointer_get 加 g_object_ref（g_autoptr）持有临时引用，保证并发刷新期间读者安全 - 3 秒低优先级延迟 unref 仅用于覆盖 load 到 ref 的指令间隙；依赖宿主迭代默认 GLib 主上下文（Qt GUI 宿主满足），批量刷新时会短暂驻留被替换的 GFileInfo Log: 修复排序线程并发刷新/读取文件属性时 gfileinfo 无锁替换导致的随机崩溃（堆损坏延迟爆发在 malloc），ASAN 压测原可复现 heap-use-after-free，修复后 60 秒满载并发压测零报错 Task: https://pms.uniontech.com/task-view-395127.html
Reviewer's GuideReplaces the unlocked GFileInfo lifecycle with atomic pointer swaps, delayed retirement, and per-read reference snapshots, preventing refresh/read use-after-free and fixing async replacement leaks. Sequence diagram for safe GFileInfo refresh and attribute readsequenceDiagram
participant Refresh as Refresh thread
participant Slot as gfileinfo slot
participant Reader as Attribute reader
participant GLib as GLib main context
Refresh->>Slot: replaceGFileInfo(fileinfo)
Slot-->>Refresh: Atomic CAS exchange
Slot->>GLib: g_timeout_add_seconds_full(3, retireUnrefCb, old)
Reader->>Slot: atomicLoadGFileInfo()
Slot-->>Reader: Current GFileInfo*
Reader->>Reader: refGFileInfo()
Reader->>Reader: Read attributes with owned reference
Reader->>Reader: g_autoptr unrefs snapshot
GLib->>GLib: retireUnrefCb(old)
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
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/dfm-io/dfm-io/dfileinfo.cpp" line_range="278" />
<code_context>
- this->gfileinfo = nullptr;
- }
- this->gfileinfo = fileinfo;
+ replaceGFileInfo(&this->gfileinfo, fileinfo);
initFinished = true;
isQuquerying = false;
</code_context>
<issue_to_address>
**issue (bug_risk):** replaceGFileInfo() updates gfileinfo atomically, but queryInfoSync(), queryInfoAsync(), and initQuerierAsync() still read the same member with ordinary non-atomic accesses. A concurrent refresh therefore creates a C++ data race and undefined behavior despite the new atomic writer.
**Triggers:** When a refresh callback or queryInfoSync() replaces gfileinfo concurrently with another initialization/query path checking the member.
**Suggested fix:** Use atomicLoadGFileInfo() for every gfileinfo read, including the early-exit checks, and make destruction/initial construction follow the same ownership protocol.
</issue_to_address>
### Comment 2
<location path="src/dfm-io/dfm-io/dfileinfo.cpp" line_range="33-50" />
<code_context>
+ return G_SOURCE_REMOVE;
+}
+
+static GFileInfo *atomicLoadGFileInfo(GFileInfo *const *slot)
+{
+ return static_cast<GFileInfo *>(g_atomic_pointer_get(reinterpret_cast<const volatile gpointer *>(slot)));
+}
+
+static GFileInfo *refGFileInfo(GFileInfo *info)
+{
+ return info ? static_cast<GFileInfo *>(g_object_ref(info)) : nullptr;
+}
+
+static void replaceGFileInfo(GFileInfo **slot, GFileInfo *value)
+{
+ GFileInfo *old = nullptr;
+ do {
+ old = atomicLoadGFileInfo(slot);
+ } while (!g_atomic_pointer_compare_and_exchange(reinterpret_cast<volatile gpointer *>(slot), old, value));
+ if (old)
+ g_timeout_add_seconds_full(G_PRIORITY_LOW, 3, retireUnrefCb, old, nullptr);
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The reader performs a load followed by g_object_ref, while replaceGFileInfo() schedules the old object's unref after a fixed three seconds. If a reader is descheduled or otherwise delayed longer than three seconds between those instructions, retireUnrefCb() frees the object before g_object_ref() executes, so the reader still dereferences freed memory.
**Triggers:** When a reader thread is paused for at least three seconds after atomicLoadGFileInfo() returns and before refGFileInfo() increments the reference count.
**Suggested fix:** Use a reclamation scheme that guarantees the loaded pointer remains alive until the reader has referenced it, such as a mutex, hazard pointers, epochs, or an atomic reference-count acquisition protocol.
</issue_to_address>
### Comment 3
<location path="src/dfm-io/dfm-io/dfileinfo.cpp" line_range="50" />
<code_context>
+ old = atomicLoadGFileInfo(slot);
+ } while (!g_atomic_pointer_compare_and_exchange(reinterpret_cast<volatile gpointer *>(slot), old, value));
+ if (old)
+ g_timeout_add_seconds_full(G_PRIORITY_LOW, 3, retireUnrefCb, old, nullptr);
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Every replacement with a non-null old pointer queues an unref on the default GLib main context. In a host that does not iterate that context, the timeout never runs and one retired GFileInfo remains referenced for every refresh, causing unbounded memory growth.
**Triggers:** When this library is used by a CLI, worker, or other host without a running default GLib main loop.
**Suggested fix:** Provide an explicit reclamation path independent of the default main context, or document and enforce a main-context owner and synchronously drain/cancel retired references during teardown.
</issue_to_address>| this->gfileinfo = nullptr; | ||
| } | ||
| this->gfileinfo = fileinfo; | ||
| replaceGFileInfo(&this->gfileinfo, fileinfo); |
There was a problem hiding this comment.
issue (bug_risk): replaceGFileInfo() updates gfileinfo atomically, but queryInfoSync(), queryInfoAsync(), and initQuerierAsync() still read the same member with ordinary non-atomic accesses. A concurrent refresh therefore creates a C++ data race and undefined behavior despite the new atomic writer.
Triggers: When a refresh callback or queryInfoSync() replaces gfileinfo concurrently with another initialization/query path checking the member.
Suggested fix: Use atomicLoadGFileInfo() for every gfileinfo read, including the early-exit checks, and make destruction/initial construction follow the same ownership protocol.
| static GFileInfo *atomicLoadGFileInfo(GFileInfo *const *slot) | ||
| { | ||
| return static_cast<GFileInfo *>(g_atomic_pointer_get(reinterpret_cast<const volatile gpointer *>(slot))); | ||
| } | ||
|
|
||
| static GFileInfo *refGFileInfo(GFileInfo *info) | ||
| { | ||
| return info ? static_cast<GFileInfo *>(g_object_ref(info)) : nullptr; | ||
| } | ||
|
|
||
| static void replaceGFileInfo(GFileInfo **slot, GFileInfo *value) | ||
| { | ||
| GFileInfo *old = nullptr; | ||
| do { | ||
| old = atomicLoadGFileInfo(slot); | ||
| } while (!g_atomic_pointer_compare_and_exchange(reinterpret_cast<volatile gpointer *>(slot), old, value)); | ||
| if (old) | ||
| g_timeout_add_seconds_full(G_PRIORITY_LOW, 3, retireUnrefCb, old, nullptr); |
There was a problem hiding this comment.
issue (bug_risk): The reader performs a load followed by g_object_ref, while replaceGFileInfo() schedules the old object's unref after a fixed three seconds. If a reader is descheduled or otherwise delayed longer than three seconds between those instructions, retireUnrefCb() frees the object before g_object_ref() executes, so the reader still dereferences freed memory.
Triggers: When a reader thread is paused for at least three seconds after atomicLoadGFileInfo() returns and before refGFileInfo() increments the reference count.
Suggested fix: Use a reclamation scheme that guarantees the loaded pointer remains alive until the reader has referenced it, such as a mutex, hazard pointers, epochs, or an atomic reference-count acquisition protocol.
| old = atomicLoadGFileInfo(slot); | ||
| } while (!g_atomic_pointer_compare_and_exchange(reinterpret_cast<volatile gpointer *>(slot), old, value)); | ||
| if (old) | ||
| g_timeout_add_seconds_full(G_PRIORITY_LOW, 3, retireUnrefCb, old, nullptr); |
There was a problem hiding this comment.
issue (bug_risk): Every replacement with a non-null old pointer queues an unref on the default GLib main context. In a host that does not iterate that context, the timeout never runs and one retired GFileInfo remains referenced for every refresh, causing unbounded memory growth.
Triggers: When this library is used by a CLI, worker, or other host without a running default GLib main loop.
Suggested fix: Provide an explicit reclamation path independent of the default main context, or document and enforce a main-context owner and synchronously drain/cancel retired references during teardown.
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价： 优秀 ✅ 通过 潜在问题： 建议： 语法正确，逻辑清晰。CAS 循环模式正确，g_autoptr 自动释放机制使用得当，refGFileInfo 的空指针检查完善 2. 代码质量 ✅评价： 优秀 ✅ 通过 潜在问题：
建议： 建议为 replaceGFileInfo 函数添加注释，说明 CAS 交换 + 3s 延迟 unref 的设计意图：确保读者线程在加载旧指针后仍有时间获取引用 3. 代码性能 ✅评价： 优秀 ✅ 通过 潜在问题：
建议： 性能良好，资源使用合理。原子操作开销极小，3s 延迟 unref 的内存开销在可接受范围内 4. 代码安全 🔒评价： 优秀 ✅ 通过
安全漏洞详情： 建议： 存在0个安全漏洞。本次修复消除了堆使用后释放（use-after-free）的内存安全问题，原子操作和引用计数方案安全可靠 💡 改进建议代码示例// 建议为 replaceGFileInfo 添加设计注释
static void replaceGFileInfo(GFileInfo **slot, GFileInfo *value)
{
// 原子 CAS 交换：确保多线程写入安全
// 旧 GFileInfo 通过 3s 延迟 unref 释放，
// 确保读者线程在加载旧指针后仍有时间通过 refGFileInfo 获取引用
GFileInfo *old = nullptr;
do {
old = atomicLoadGFileInfo(slot);
} while (!g_atomic_pointer_compare_and_exchange(
reinterpret_cast<volatile gpointer *>(slot), old, value));
if (old)
g_timeout_add_seconds_full(G_PRIORITY_LOW, 3, retireUnrefCb, old, nullptr);
}本报告由 AI 代码审查工具自动生成 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: liyigang1, max-lvs, 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 |
|
/merge |
66a7c7c
into
linuxdeepin:develop/meagle-20260526
4 participants
Root cause
Crash stack (dde-file-manager FileSortWorker thread, dies in
__libc_mallocviag_strdup → g_canonicalize_filename → gvfs dbus → DLocalHelper → DFileInfo::attribute): heap metadata was already corrupted by an earlier write — the real culprit isDFileInfoPrivate::gfileinfo(rawGFileInfo*) having a completely lock-free lifecycle:queryInfoSync()unrefs and replacesgfileinfowithout any lock, while reader threads concurrently callg_file_info_get_attribute_*(gfileinfo, ...)inattribute()/hasAttribute()/customAttribute()/attributesBySelf()/exists()queryInfoAsyncCallback/2) overwriteme->gfileinfodirectly (also leaking the old pointer)QMutexLockerinDFileInfo::attribute()never closes the window because the writer never takes that lockASAN reproducer (4 refresh threads + 4 attribute-reading threads sharing one DFileInfo): heap-use-after-free within seconds; freed by gio finalize on the refresh thread, used by the reader while holding the mutex.
Fix (lock-free)
queryInfoSync, both async callbacks): atomic CAS exchange viareplaceGFileInfo(), ownership transferred exactly once; the retiredGFileInfois unref'd by a 3s low-priority timeout, which only needs to cover the reader's load-to-ref instruction gapg_autoptr(GFileInfo) gfileinfo = refGFileInfo(atomicLoadGFileInfo(&...))— taking a real reference for the whole read, shadowing the member so the function bodies stay untouchedGFileInfoleak in the async callbacks where the old pointer was overwritten without unrefKnown trade-offs
Verification
ASAN build of this library + 8-thread stress reproducer, 60s full load:
GLib-GIO-CRITICALassertion spamTask: https://pms.uniontech.com/task-view-395127.html
Summary by Sourcery
Make DFileInfo GFileInfo lifecycle safe under concurrent refreshes and attribute access to eliminate heap corruption crashes.
Bug Fixes:
Enhancements:
Tests: