Re: [PATCH v3] xfs_db: release ip resource before returning from get_next_unlinked()

[Bill O'Donnell <bodonnel@redhat.com>]

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@redhat.com>
> > >> ---
> > >> 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

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