On 8/8/24 4:13 PM, Bill O'Donnell wrote:
> On Thu, Aug 08, 2024 at 02:41:39PM -0500, Bill O'Donnell wrote:
>> On Thu, Aug 08, 2024 at 02:00:22PM -0500, Eric Sandeen wrote:
>>> On 8/8/24 1:28 PM, Darrick J. Wong wrote:
>>>> On Wed, Aug 07, 2024 at 02:38:03PM -0500, Bill O'Donnell wrote:
>>>>> Fix potential memory leak in function get_next_unlinked(). Call
>>>>> libxfs_irele(ip) before exiting.
>>>>>
>>>>> Details:
>>>>> Error: RESOURCE_LEAK (CWE-772):
>>>>> xfsprogs-6.5.0/db/iunlink.c:51:2: alloc_arg: "libxfs_iget" allocates memory that is stored into "ip".
>>>>> xfsprogs-6.5.0/db/iunlink.c:68:2: noescape: Resource "&ip->i_imap" is not freed or pointed-to in "libxfs_imap_to_bp".
>>>>> xfsprogs-6.5.0/db/iunlink.c:76:2: leaked_storage: Variable "ip" going out of scope leaks the storage it points to.
>>>>> # 74| libxfs_buf_relse(ino_bp);
>>>>> # 75|
>>>>> # 76|-> return ret;
>>>>> # 77| bad:
>>>>> # 78| dbprintf(_("AG %u agino %u: %s\n"), agno, agino, strerror(error));
>>>>>
>>>>> Signed-off-by: Bill O'Donnell <bodonnel@xxxxxxxxxx>
>>>>> ---
>>>>> v2: cover error case.
>>>>> v3: fix coverage to not release unitialized variable.
>>>>> ---
>>>>> db/iunlink.c | 7 +++++--
>>>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/db/iunlink.c b/db/iunlink.c
>>>>> index d87562e3..57e51140 100644
>>>>> --- a/db/iunlink.c
>>>>> +++ b/db/iunlink.c
>>>>> @@ -66,15 +66,18 @@ get_next_unlinked(
>>>>> }
>>>>>
>>>>> error = -libxfs_imap_to_bp(mp, NULL, &ip->i_imap, &ino_bp);
>>>>> - if (error)
>>>>> + if (error) {
>>>>> + libxfs_buf_relse(ino_bp);
>>>>
>>>> Sorry, I think I've led you astray -- it's not necessary to
>>>> libxfs_buf_relse in any of the bailouts.
>>>>
>>>> --D
>>>>
>>>>> goto bad;
>>>>> -
>>>>> + }
>>>>> dip = xfs_buf_offset(ino_bp, ip->i_imap.im_boffset);
>>>>> ret = be32_to_cpu(dip->di_next_unlinked);
>>>>> libxfs_buf_relse(ino_bp);
>>>>> + libxfs_irele(ip);
>>>>>
>>>>> return ret;
>>>>> bad:
>>>>> + libxfs_irele(ip);
>>>
>>> And this addition results in a libxfs_irele of an ip() which failed iget()
>>> via the first goto bad, so you're releasing a thing which was never obtained,
>>> which doesn't make sense.
>>>
>>>
>>> There are 2 relevant actions here. The libxfs_iget, and the libxfs_imap_to_bp.
>>> Only after libxfs_iget(ip) /succeeds/ does it need a libxfs_irele(ip), on either
>>> error paths or normal exit. The fact that it does neither leads to the two leaks
>>> noted in CID 1554242.
>>
>> In libxfs_iget, -ENOMEM is returned when kmem_cache_zalloc() fails. For all other
>> error cases in that function, kmem_cache_free() releases the memory that was presumably
>> successfully allocated. I had wondered if we need to use libxfs_irele() at all in
>> get_next_unlinked() (except for the success case)?
>
> So, if that's the case, I'm back to v1 of this patch.
> -Bill
when libxfs_iget succeeds, it has obtained an inode.
After that has happened, libxfs_irele needs to be called either in the normal
return path, or any subsequent error path, to free that inode in this function.
As Darrick pointed out in his first reply, V1 missed irele on the error path,
so it was not sufficient.
-Eric
