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

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

On Fri, Aug 02, 2024 at 04:23:00PM -0700, Darrick J. Wong wrote:
> On Fri, Aug 02, 2024 at 05:25:52PM -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>
> > ---
> >  db/iunlink.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/db/iunlink.c b/db/iunlink.c
> > index d87562e3..3b2417c5 100644
> > --- a/db/iunlink.c
> > +++ b/db/iunlink.c
> > @@ -72,6 +72,7 @@ get_next_unlinked(
> >  	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);
> 
> I think this needs to cover the error return for libxfs_imap_to_bp too,
> doesn't it?

I considered that, but there are several places in the code where the
error return doesn't release the resource. Not that that's correct, but the
scans didn't flag them. For example, in bmap_inflate.c, bmapinflate_f()
does not release the resource and scans didn't flag it.

Thanks-
Bill


> 
> --D
> 
> >  
> >  	return ret;
> >  bad:
> > -- 
> > 2.45.2
> > 
>