Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH] hfs: fix a memleak in hfs_find_init | From | Viacheslav Dubeyko <> | Date | Mon, 29 Jan 2024 17:25:26 +0300 |
| |
> 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.
| ![\](/images/icornerr.gif) |