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

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

On Mon, Aug 05, 2024 at 09:42:15AM -0500, Bill O'Donnell wrote:
> 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.

Looking at libxfs_iget(), it seems that for error cases, the resource is
released within that function.
-Bill

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