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

[PATCH v5] 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.
v4: add logic to cover error case when ip is not attained.
v5: no need to release a NULL tp in the first error case.
---
 db/iunlink.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/db/iunlink.c b/db/iunlink.c
index d87562e3..35dbc3a0 100644
--- a/db/iunlink.c
+++ b/db/iunlink.c
@@ -66,12 +66,15 @@ get_next_unlinked(
 	}
 
 	error = -libxfs_imap_to_bp(mp, NULL, &ip->i_imap, &ino_bp);
-	if (error)
+	if (error) {
+		libxfs_irele(ip);
 		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:
-- 
2.46.0

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

[Eric Sandeen]

On 8/9/24 12:15 PM, 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>

I think this looks ok now. I might have added a new irele: target
and goto irele; so it's not mixing unwinding styles, but I think that's
just personal preference in this relatively simple function.

Reviewed-by: Eric Sandeen <sandeen@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.
> v5: no need to release a NULL tp in the first error case.
> ---
>  db/iunlink.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/db/iunlink.c b/db/iunlink.c
> index d87562e3..35dbc3a0 100644
> --- a/db/iunlink.c
> +++ b/db/iunlink.c
> @@ -66,12 +66,15 @@ get_next_unlinked(
>  	}
>  
>  	error = -libxfs_imap_to_bp(mp, NULL, &ip->i_imap, &ino_bp);
> -	if (error)
> +	if (error) {
> +		libxfs_irele(ip);
>  		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:

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

[Darrick J. Wong]

On Fri, Aug 09, 2024 at 12:15:42PM -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>

Looks good to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
> v2: cover error case.
> v3: fix coverage to not release unitialized variable.
> v4: add logic to cover error case when ip is not attained.
> v5: no need to release a NULL tp in the first error case.
> ---
>  db/iunlink.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/db/iunlink.c b/db/iunlink.c
> index d87562e3..35dbc3a0 100644
> --- a/db/iunlink.c
> +++ b/db/iunlink.c
> @@ -66,12 +66,15 @@ get_next_unlinked(
>  	}
>  
>  	error = -libxfs_imap_to_bp(mp, NULL, &ip->i_imap, &ino_bp);
> -	if (error)
> +	if (error) {
> +		libxfs_irele(ip);
>  		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:
> -- 
> 2.46.0
> 
>