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

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

On Fri, Aug 09, 2024 at 09:23:26AM -0700, Darrick J. Wong wrote:
> On Fri, Aug 09, 2024 at 11:15:11AM -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.
> > v4: add logic to cover error case when ip is not attained.
> > ---
> > db/iunlink.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/db/iunlink.c b/db/iunlink.c
> > index d87562e3..98d1effc 100644
> > --- a/db/iunlink.c
> > +++ b/db/iunlink.c
> > @@ -49,8 +49,12 @@ get_next_unlinked(
> >  
> >  	ino = XFS_AGINO_TO_INO(mp, agno, agino);
> >  	error = -libxfs_iget(mp, NULL, ino, 0, &ip);
> > -	if (error)
> > -		goto bad;
> > +	if (error) {
> > +		if (ip)
> > +			goto bad_rele;
> 
> When does libxfs_iget return nonzero and a non-NULL ip?  Wouldn't 'goto
> bad' suffice here?

Yes, and I thought that in v1. My mistake here. I'll send a v5.
-Bill

> 
> --D
> 
> > +		else
> > +			goto bad;
> > +	}
> >  
> >  	if (verbose) {
> >  		xfs_filblks_t	blocks, rtblks = 0;
> > @@ -67,13 +71,16 @@ get_next_unlinked(
> >  
> >  	error = -libxfs_imap_to_bp(mp, NULL, &ip->i_imap, &ino_bp);
> >  	if (error)
> > -		goto bad;
> > +		goto bad_rele;
> >  
> >  	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_rele:
> > +	libxfs_irele(ip);
> >  bad:
> >  	dbprintf(_("AG %u agino %u: %s\n"), agno, agino, strerror(error));
> >  	return NULLAGINO;
> > -- 
> > 2.46.0
> > 
> > 
>