fix:[dfsv1][elm]Fixed a memory leak in dfs_elm_close #10868
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: 2025-10-29 13:42 CST)
📝 Review Instructions
|
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes a memory leak in the elmfat filesystem driver by removing an early return in the dfs_elm_close function that was preventing proper resource cleanup when multiple file descriptors referenced the same vnode.
Key changes:
- Removed ref_count check that caused early return in
dfs_elm_close - Added assertion to validate
file->datais not NULL - Enhanced Kconfig documentation for exFAT support with detailed usage guidelines
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| components/dfs/dfs_v1/filesystems/elmfat/dfs_elm.c | Fixed memory leak by removing early return based on ref_count check and added data validation assertion |
| components/dfs/Kconfig | Improved exFAT configuration option description with comprehensive usage guidelines and recommendations |
Comments suppressed due to low confidence (1)
components/dfs/dfs_v1/filesystems/elmfat/dfs_elm.c:494
- bug/错误: The fix introduces a more serious bug than the memory leak it attempts to solve. By removing the ref_count check,
dfs_elm_closenow unconditionally freesfile->dataon every close call. However, when multiple file descriptors share the same vnode (ref_count > 1), they may share the samefile->datapointer. This means the first close will free the memory, and subsequent closes will trigger use-after-free or double-free errors, leading to crashes or memory corruption.
English: The original code prevented cleanup when ref_count > 1 to avoid this exact problem. The actual memory leak fix requires a different approach: the reference counting mechanism should be managed at the DFS layer (in dfs_file.c), which already handles vnode cleanup when ref_count reaches 0. The filesystem-specific close function should only be called once when the last reference is released. The current change breaks this contract and will cause memory corruption in scenarios where files are opened multiple times (e.g., via dup() or multiple open() calls to the same path).
中文：原始代码在 ref_count > 1 时阻止清理正是为了避免这个问题。实际的内存泄漏修复需要采用不同的方法:引用计数机制应该在DFS层(在 dfs_file.c 中)管理,该层已经在 ref_count 达到0时处理vnode清理。文件系统特定的close函数应该只在最后一个引用被释放时调用一次。当前的更改破坏了这个约定,并且会在文件被多次打开的场景中(例如通过dup()或对同一路径的多次open()调用)导致内存损坏。
Recommendation/建议:
int dfs_elm_close(struct dfs_file *file)
{
FRESULT result;
RT_ASSERT(file->vnode->ref_count > 0);
if (file->vnode->ref_count > 1)
{
return 0;
}
/* Only cleanup when this is the last reference */
result = FR_OK;
if (file->vnode->type == FT_DIRECTORY)
{
DIR *dir = RT_NULL;
dir = (DIR *)(file->data);
RT_ASSERT(dir != RT_NULL);
/* release memory */
rt_free(dir);
}
else if (file->vnode->type == FT_REGULAR)
{
FIL *fd = RT_NULL;
fd = (FIL *)(file->data);
RT_ASSERT(fd != RT_NULL);
result = f_close(fd);
/* release memory */
rt_free(fd);
}
return elm_result_to_dfs(result);
} RT_ASSERT(file->vnode->ref_count > 0);
RT_ASSERT(file->data != RT_NULL);
result = FR_OK;
if (file->vnode->type == FT_DIRECTORY)
{
DIR *dir = RT_NULL;
dir = (DIR *)(file->data);
RT_ASSERT(dir != RT_NULL);
/* release memory */
rt_free(dir);
}
else if (file->vnode->type == FT_REGULAR)
{
FIL *fd = RT_NULL;
fd = (FIL *)(file->data);
RT_ASSERT(fd != RT_NULL);
result = f_close(fd);
/* release memory */
rt_free(fd);
}
| RT_ASSERT(file->vnode->ref_count > 0); | ||
| if (file->vnode->ref_count > 1) | ||
| { | ||
| return 0; | ||
| } | ||
| RT_ASSERT(file->data != RT_NULL); |
There was a problem hiding this comment.
bug/错误: The assertion file->data != RT_NULL should use file->data != NULL for consistency with the rest of the codebase. RT-Thread code typically uses RT_NULL for RT-Thread API pointer parameters but NULL for standard C pointer checks. However, more critically, this assertion may be too strict - when ref_count > 1, the file->data could legitimately be NULL since the dfs_elm_open function only allocates file->data when ref_count == 1 (line 345-353 shows early return when ref_count > 1, and data allocation happens later). This assertion could cause crashes in valid multi-reference scenarios.
English: The assertion on line 472 is problematic. When vnode->ref_count > 1, multiple file descriptors share the same vnode but may have different file->data values. The dfs_elm_open function only allocates memory for file->data when opening a new file (ref_count == 1), but returns early when ref_count > 1. This means that for duplicate file descriptors, file->data might not be allocated, making this assertion incorrect and potentially causing crashes.
中文：第472行的断言存在问题。当 vnode->ref_count > 1 时,多个文件描述符共享同一个vnode但可能有不同的 file->data 值。dfs_elm_open 函数仅在打开新文件时(ref_count == 1)为 file->data 分配内存,但在 ref_count > 1 时会提前返回。这意味着对于重复的文件描述符,file->data 可能未被分配,使得此断言不正确并可能导致崩溃。
2 participants
拉取/合并请求描述：(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