| Message ID | 20240122172719.3843098-1-alexious@zju.edu.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hfs: fix a memleak in hfs_find_init | expand |
> On 22 Jan 2024, at 20:27, Zhipeng Lu <alexious@zju.edu.cn> wrote: > > When the switch statment goes to default and return an error, ptr should > be freed since it is allocated in hfs_find_init. > Do you have any memory leaks report? Could you share it in the comments? Which use-case reproduces the issue? It will be easier to review the fix If you can share the path of reproduction. Thanks, Slava. > Fixes: b3b2177a2d79 ("hfs: add lock nesting notation to hfs_find_init") > Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> > --- > fs/hfs/bfind.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c > index ef9498a6e88a..7aa3b9aba4d1 100644 > --- a/fs/hfs/bfind.c > +++ b/fs/hfs/bfind.c > @@ -36,6 +36,7 @@ int hfs_find_init(struct hfs_btree *tree, struct hfs_find_data *fd) > mutex_lock_nested(&tree->tree_lock, ATTR_BTREE_MUTEX); > break; > default: > + kfree(fd->search_key); > return -EINVAL; > } > return 0; > -- > 2.34.1 >
> > On 22 Jan 2024, at 20:27, Zhipeng Lu <alexious@zju.edu.cn> wrote: > > > > When the switch statment goes to default and return an error, ptr should > > be freed since it is allocated in hfs_find_init. > > > > Do you have any memory leaks report? Could you share it in the comments? > Which use-case reproduces the issue? It will be easier to review the fix > If you can share the path of reproduction. > > Thanks, > Slava. Well, we found this potential memory leak by static analysis. We found that all of hfs_find_init's callers won't release `ptr` when hfs_find_init fails, while they will do release `ptr` when functions that after hfs_find_init fails. This tactic observation suggests that hfs_find_init proberly should release `ptr` when it fails, i.e. in the default branch of switch in this patch. Besides, we noticed another implementation of hfs_find_init in fs/hfsplus/bfind.c, which is essentially identical to the one in this patch (in fs/hfs/bfind.c) but calling `BUG();` in default branch to trigger an error-handling. Thanks, Zhipeng. > > > Fixes: b3b2177a2d79 ("hfs: add lock nesting notation to hfs_find_init") > > Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> > > --- > > fs/hfs/bfind.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c > > index ef9498a6e88a..7aa3b9aba4d1 100644 > > --- a/fs/hfs/bfind.c > > +++ b/fs/hfs/bfind.c > > @@ -36,6 +36,7 @@ int hfs_find_init(struct hfs_btree *tree, struct hfs_find_data *fd) > > mutex_lock_nested(&tree->tree_lock, ATTR_BTREE_MUTEX); > > break; > > default: > > + kfree(fd->search_key); > > return -EINVAL; > > } > > return 0; > > -- > > 2.34.1 > >
> On 29 Jan 2024, at 15:54, alexious@zju.edu.cn wrote: > >>> On 22 Jan 2024, at 20:27, Zhipeng Lu <alexious@zju.edu.cn> wrote: >>> >>> When the switch statment goes to default and return an error, ptr should >>> be freed since it is allocated in hfs_find_init. >>> >> >> Do you have any memory leaks report? Could you share it in the comments? >> Which use-case reproduces the issue? It will be easier to review the fix >> If you can share the path of reproduction. >> >> Thanks, >> Slava. > > Well, we found this potential memory leak by static analysis. > > We found that all of hfs_find_init's callers won't release `ptr` when > hfs_find_init fails, while they will do release `ptr` when functions > that after hfs_find_init fails. This tactic observation suggests that > hfs_find_init proberly should release `ptr` when it fails, i.e. in the > default branch of switch in this patch. > > Besides, we noticed another implementation of hfs_find_init in > fs/hfsplus/bfind.c, which is essentially identical to the one in > this patch (in fs/hfs/bfind.c) but calling `BUG();` in default branch > to trigger an error-handling. > I see. I believe it makes sense to add all of this explanation into comment section. Modification looks good. Mostly, hfs_find_exit() does freeing resources and if hfs_find_init() fails, then hfs_find_exit() is never called. Maybe, it makes sense to set fd->tree = NULL too but it is not critical, as far as I can see. Could you please rework the comment section of the patch? Thanks, Slava.
> > On 29 Jan 2024, at 15:54, alexious@zju.edu.cn wrote: > > > >>> On 22 Jan 2024, at 20:27, Zhipeng Lu <alexious@zju.edu.cn> wrote: > >>> > >>> When the switch statment goes to default and return an error, ptr should > >>> be freed since it is allocated in hfs_find_init. > >>> > >> > >> Do you have any memory leaks report? Could you share it in the comments? > >> Which use-case reproduces the issue? It will be easier to review the fix > >> If you can share the path of reproduction. > >> > >> Thanks, > >> Slava. > > > > Well, we found this potential memory leak by static analysis. > > > > We found that all of hfs_find_init's callers won't release `ptr` when > > hfs_find_init fails, while they will do release `ptr` when functions > > that after hfs_find_init fails. This tactic observation suggests that > > hfs_find_init proberly should release `ptr` when it fails, i.e. in the > > default branch of switch in this patch. > > > > Besides, we noticed another implementation of hfs_find_init in > > fs/hfsplus/bfind.c, which is essentially identical to the one in > > this patch (in fs/hfs/bfind.c) but calling `BUG();` in default branch > > to trigger an error-handling. > > > > I see. I believe it makes sense to add all of this explanation > into comment section. Modification looks good. Mostly, hfs_find_exit() > does freeing resources and if hfs_find_init() fails, then hfs_find_exit() > is never called. Maybe, it makes sense to set fd->tree = NULL too but > it is not critical, as far as I can see. > > Could you please rework the comment section of the patch? Sure, I'll including such idea in this patch and send a v2 version of this patch. Thanks, Zhipeng. > Thanks, > Slava.
diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c index ef9498a6e88a..7aa3b9aba4d1 100644 --- a/fs/hfs/bfind.c +++ b/fs/hfs/bfind.c @@ -36,6 +36,7 @@ int hfs_find_init(struct hfs_btree *tree, struct hfs_find_data *fd) mutex_lock_nested(&tree->tree_lock, ATTR_BTREE_MUTEX); break; default: + kfree(fd->search_key); return -EINVAL; } return 0;
When the switch statment goes to default and return an error, ptr should be freed since it is allocated in hfs_find_init. Fixes: b3b2177a2d79 ("hfs: add lock nesting notation to hfs_find_init") Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> --- fs/hfs/bfind.c | 1 + 1 file changed, 1 insertion(+)