fix[dfs_v1]: prevent vnode ref underflow and double release on close/fd release #11119
Conversation
|
👋 感谢您对 RT-Thread 的贡献！Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范，请在你的仓库中执行以下步骤运行代码格式化工作流（如果格式化CI运行失败）。 🛠 操作步骤 | Steps
完成后，提交将自动更新至 如有问题欢迎联系我们，再次感谢您的贡献！💐 |
📌 Code Review Assignment🏷️ Tag: componentsReviewers: @Maihuanyi Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2026-01-05 10:46 CST)
📝 Review Instructions
|
|
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix critical reference counting bugs in the DFS v1 filesystem that could lead to vnode reference count underflow and potential double-free vulnerabilities (as reported in issues #10847 and #10848). The changes focus on adding NULL checks and reordering operations to prevent improper vnode cleanup.
Key changes:
- Added NULL check for vnode before accessing ref_count in
dfs_file_close - Reordered vnode ref_count decrement and cleanup logic in
dfs_file_close - Added underflow protection and moved vnode NULL assignment earlier in
fdt_fd_release
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| components/dfs/dfs_v1/src/dfs_file.c | Modified dfs_file_close to add NULL check for vnode, decrement ref_count before checking if zero, and moved fd->vnode = NULL assignment before cleanup |
| components/dfs/dfs_v1/src/dfs.c | Modified fdt_fd_release to add underflow guard, move vnode NULL assignment earlier, and add ref_count check before decrement |
| if (vnode->ref_count > 0) | ||
| { | ||
| vnode->ref_count--; | ||
| } |
There was a problem hiding this comment.
[Concurrency/并发]: Insufficient synchronization between dfs_file_close and fdt_fd_release / dfs_file_close 和 fdt_fd_release 之间的同步不足
English: The dfs_file_close function uses dfs_fm_lock() (vnode manager lock) while fdt_fd_release uses dfs_file_lock() (fd table lock). These are two different locks protecting different resources. This creates a race condition where:
- Thread A calls
dfs_file_close, decrementsvnode->ref_countto 0, and starts cleaning up - Thread B calls
fdt_fd_releaseconcurrently and also tries to decrement the same vnode's ref_count - Both threads might try to free the same vnode, or one might access freed memory
The guard if (vnode->ref_count > 0) at line 418 only partially mitigates this - if the race occurs after the check but before the decrement, underflow can still happen.
中文：dfs_file_close 函数使用 dfs_fm_lock()（vnode 管理器锁），而 fdt_fd_release 使用 dfs_file_lock()（fd 表锁）。这是两个不同的锁保护不同的资源。这会创建一个竞争条件：
- 线程 A 调用
dfs_file_close，将vnode->ref_count递减到 0，并开始清理 - 线程 B 同时调用
fdt_fd_release，也尝试递减同一个 vnode 的 ref_count - 两个线程可能都尝试释放同一个 vnode，或者一个可能访问已释放的内存
第 418 行的保护条件 if (vnode->ref_count > 0) 只能部分缓解这个问题 - 如果竞争发生在检查之后但递减之前，仍然可能发生下溢。
A proper fix would require using the same lock (likely dfs_fm_lock) in both functions when accessing vnode ref_count.
| vnode->ref_count--; | ||
| fd->vnode = NULL; | ||
| if (vnode->ref_count == 0) |
There was a problem hiding this comment.
[Bug/错误]: Reference count decrement and vnode cleanup order may cause race condition / 引用计数递减和vnode清理顺序可能导致竞争条件
English: The current implementation decrements vnode->ref_count and sets fd->vnode = NULL before checking if ref_count == 0. This creates a race condition window where another thread could access the vnode with ref_count=0 but before it's cleaned up. The check should be performed on the original ref_count value before decrementing, not after.
中文：当前实现在检查 ref_count == 0 之前先递减 vnode->ref_count 并设置 fd->vnode = NULL。这会创建一个竞争条件窗口，其中另一个线程可能在清理之前访问 ref_count=0 的 vnode。应该在递减之前对原始 ref_count 值执行检查，而不是之后。
The logic should be:
- Check if
vnode->ref_count == 1(last reference) - If yes, perform cleanup (remove from hash, free memory)
- Otherwise, just decrement
ref_count - Finally, set
fd->vnode = NULL
逻辑应该是：
- 检查
vnode->ref_count == 1（最后一个引用） - 如果是，执行清理（从哈希中删除，释放内存）
- 否则，只需递减
ref_count - 最后，设置
fd->vnode = NULL
| if (vnode->ref_count > 0) | ||
| { | ||
| vnode->ref_count--; | ||
| } | ||
| if(vnode->ref_count == 0) | ||
| { | ||
| rt_free(vnode); | ||
| fd_slot->vnode = RT_NULL; | ||
| } |
There was a problem hiding this comment.
[Bug/错误]: Potential vnode double-free vulnerability / 潜在的 vnode 双重释放漏洞
English: While the guard if (vnode->ref_count > 0) prevents underflow, the logic here is problematic. If fdt_fd_release is called when the vnode was already freed by dfs_file_close, this could lead to use-after-free. The vnode might have already been freed in dfs_file_close at line 363, but fd_slot->vnode still holds a dangling pointer until line 415 sets it to NULL. Additionally, freeing the vnode here at line 424 without proper cleanup (removing from hash list, freeing path/fullpath) is incomplete compared to the cleanup in dfs_file_close. This inconsistency suggests a design flaw in managing vnode lifecycle across two different code paths.
中文：虽然保护条件 if (vnode->ref_count > 0) 可以防止下溢，但这里的逻辑存在问题。如果在 vnode 已被 dfs_file_close 释放后调用 fdt_fd_release，这可能导致释放后使用。vnode 可能已在 dfs_file_close 的第 363 行被释放，但 fd_slot->vnode 仍持有悬空指针，直到第 415 行将其设置为 NULL。此外，在第 424 行释放 vnode 时没有适当的清理（从哈希列表中删除、释放 path/fullpath），与 dfs_file_close 中的清理相比是不完整的。这种不一致性表明在两个不同代码路径中管理 vnode 生命周期存在设计缺陷。
The correct approach should ensure that vnode cleanup is performed in only one place with proper synchronization, or the cleanup in both places should be identical and complete.
| { | ||
| vnode->ref_count--; | ||
| } | ||
| if(vnode->ref_count == 0) |
There was a problem hiding this comment.
[Code Style/代码风格]: Missing space after 'if' keyword / 'if' 关键字后缺少空格
English: According to RT-Thread coding standards, there should be a space between 'if' and the opening parenthesis. The code should be if (vnode->ref_count == 0) instead of if(vnode->ref_count == 0).
中文：根据 RT-Thread 编码标准，'if' 和左括号之间应该有一个空格。代码应该是 if (vnode->ref_count == 0) 而不是 if(vnode->ref_count == 0)。
| if(vnode->ref_count == 0) | |
| if (vnode->ref_count == 0) |
| if (vnode == NULL || vnode->ref_count <= 0) | ||
| { | ||
| dfs_fm_unlock(); | ||
| return -ENXIO; |
There was a problem hiding this comment.
[Bug/错误]: NULL check added but early return prevents vnode cleanup / 添加了 NULL 检查但提前返回阻止了 vnode 清理
English: While adding vnode == NULL check is good defensive programming, returning -ENXIO when vnode is NULL causes a potential resource leak. If vnode is somehow NULL but fd->ref_count == 1, the function returns early without decrementing fd->ref_count or cleaning up the fd structure. This could lead to fd resources not being properly released. Consider whether this is an assertion-worthy condition instead, since vnode should never be NULL when fd->ref_count == 1 in a correctly functioning system.
中文：虽然添加 vnode == NULL 检查是良好的防御性编程，但当 vnode 为 NULL 时返回 -ENXIO 会导致潜在的资源泄漏。如果 vnode 不知何故为 NULL 但 fd->ref_count == 1，函数会提前返回，而不递减 fd->ref_count 或清理 fd 结构。这可能导致 fd 资源无法正确释放。考虑这是否应该是断言级别的条件，因为在正常运行的系统中，当 fd->ref_count == 1 时 vnode 不应该为 NULL。
1 participant
拉取/合并请求描述：(PR description)
[
为什么提交这份PR (why to submit this PR)
你的解决方案是什么 (what is your solution)
请提供验证的bsp和config (provide the config and bsp)
]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality：
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up