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.
libxfs_imap_to_bp needs a corresponding libxfs_buf_relse() (thanks for clarifying
djwong) but that libxfs_buf_relse() is already present if libxfs_imap_to_bp
succeeds. It's not needed if it fails, because there's nothing to release.
When Darrick said
> I think this needs to cover the error return for libxfs_imap_to_bp too,
> doesn't it?
I think he meant that in the error case where libxfs_imap_to_bp fails, libxfs_irele
is also needed. (In addition to being needed on a normal return.)
-Eric
