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

[PATCH v3] 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>
---
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);
 		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);
 	dbprintf(_("AG %u agino %u: %s\n"), agno, agino, strerror(error));
 	return NULLAGINO;
 }
-- 
2.45.2

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

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Darrick J. Wong]

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);
>  	dbprintf(_("AG %u agino %u: %s\n"), agno, agino, strerror(error));
>  	return NULLAGINO;
>  }
> -- 
> 2.45.2
> 
> 

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

[Eric Sandeen]

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.

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

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

[Bill O'Donnell]

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


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

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

[Bill O'Donnell]

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

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

[Eric Sandeen]

On 8/8/24 4:13 PM, Bill O'Donnell wrote:
> 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

when libxfs_iget succeeds, it has obtained an inode.

After that has happened, libxfs_irele needs to be called either in the normal
return path, or any subsequent error path, to free that inode in this function.

As Darrick pointed out in his first reply, V1 missed irele on the error path,
so it was not sufficient.

-Eric