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

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

[Bill O'Donnell]

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);
 
 	return ret;
 bad:
-- 
2.45.2

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

[Darrick J. Wong]

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?

--D

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

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

[Bill O'Donnell]

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

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

[Bill O'Donnell]

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

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

[Eric Sandeen]

On 8/5/24 9:42 AM, 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.

Upstream coverity scan does flag it, CID 1554242 (that CID actually covers
both instances of the leak).

-Eric

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

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

[Bill O'Donnell]

On Wed, Aug 07, 2024 at 12:07:37PM -0500, Eric Sandeen wrote:
> On 8/5/24 9:42 AM, 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.
> 
> Upstream coverity scan does flag it, CID 1554242 (that CID actually covers
> both instances of the leak).

Ah, thanks. I'll send a v2.
-Bill


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