Received-SPF: pass (google.com: domain of linux-kernel+bounces-42894-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Content-Type: text/plain;
	charset=us-ascii
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.400.31\))
Subject: Re: [PATCH] hfs: fix a memleak in hfs_find_init
From: Viacheslav Dubeyko <slava@dubeyko.com>
In-Reply-To: <38fd9f3.214a.18d5548ac0c.Coremail.alexious@zju.edu.cn>
Date: Mon, 29 Jan 2024 17:25:26 +0300
Cc: Andrew Morton <akpm@linux-foundation.org>,
 Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>,
 linux-fsdevel@vger.kernel.org,
 linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 7bit
Message-Id: <DCBF7671-E982-49F2-8687-DF030D2A99C7@dubeyko.com>
References: <20240122172719.3843098-1-alexious@zju.edu.cn>
 <F36C0C80-DAF3-4D8F-8EA3-5209E8FB5BE3@dubeyko.com>
 <38fd9f3.214a.18d5548ac0c.Coremail.alexious@zju.edu.cn>
To: Zhipeng Lu <alexious@zju.edu.cn>


> 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.