* [PATCH v4] xfs_db: release ip resource before returning from get_next_unlinked()
@ 2024-08-09 16:15 Bill O'Donnell
2024-08-09 16:23 ` Darrick J. Wong
0 siblings, 1 reply; 3+ messages in thread
From: Bill O'Donnell @ 2024-08-09 16:15 UTC (permalink / raw)
To: linux-xfs; +Cc: djwong, sandeen, cem, 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.
---
db/iunlink.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/db/iunlink.c b/db/iunlink.c
index d87562e3..98d1effc 100644
--- a/db/iunlink.c
+++ b/db/iunlink.c
@@ -49,8 +49,12 @@ get_next_unlinked(
ino = XFS_AGINO_TO_INO(mp, agno, agino);
error = -libxfs_iget(mp, NULL, ino, 0, &ip);
- if (error)
- goto bad;
+ if (error) {
+ if (ip)
+ goto bad_rele;
+ else
+ goto bad;
+ }
if (verbose) {
xfs_filblks_t blocks, rtblks = 0;
@@ -67,13 +71,16 @@ get_next_unlinked(
error = -libxfs_imap_to_bp(mp, NULL, &ip->i_imap, &ino_bp);
if (error)
- goto bad;
+ goto bad_rele;
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_rele:
+ libxfs_irele(ip);
bad:
dbprintf(_("AG %u agino %u: %s\n"), agno, agino, strerror(error));
return NULLAGINO;
--
2.46.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v4] xfs_db: release ip resource before returning from get_next_unlinked()
2024-08-09 16:15 [PATCH v4] xfs_db: release ip resource before returning from get_next_unlinked() Bill O'Donnell
@ 2024-08-09 16:23 ` Darrick J. Wong
2024-08-09 17:03 ` Bill O'Donnell
0 siblings, 1 reply; 3+ messages in thread
From: Darrick J. Wong @ 2024-08-09 16:23 UTC (permalink / raw)
To: Bill O'Donnell; +Cc: linux-xfs, sandeen, cem
On Fri, Aug 09, 2024 at 11:15:11AM -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.
> v4: add logic to cover error case when ip is not attained.
> ---
> db/iunlink.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/db/iunlink.c b/db/iunlink.c
> index d87562e3..98d1effc 100644
> --- a/db/iunlink.c
> +++ b/db/iunlink.c
> @@ -49,8 +49,12 @@ get_next_unlinked(
>
> ino = XFS_AGINO_TO_INO(mp, agno, agino);
> error = -libxfs_iget(mp, NULL, ino, 0, &ip);
> - if (error)
> - goto bad;
> + if (error) {
> + if (ip)
> + goto bad_rele;
When does libxfs_iget return nonzero and a non-NULL ip? Wouldn't 'goto
bad' suffice here?
--D
> + else
> + goto bad;
> + }
>
> if (verbose) {
> xfs_filblks_t blocks, rtblks = 0;
> @@ -67,13 +71,16 @@ get_next_unlinked(
>
> error = -libxfs_imap_to_bp(mp, NULL, &ip->i_imap, &ino_bp);
> if (error)
> - goto bad;
> + goto bad_rele;
>
> 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_rele:
> + libxfs_irele(ip);
> bad:
> dbprintf(_("AG %u agino %u: %s\n"), agno, agino, strerror(error));
> return NULLAGINO;
> --
> 2.46.0
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v4] xfs_db: release ip resource before returning from get_next_unlinked()
2024-08-09 16:23 ` Darrick J. Wong
@ 2024-08-09 17:03 ` Bill O'Donnell
0 siblings, 0 replies; 3+ messages in thread
From: Bill O'Donnell @ 2024-08-09 17:03 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, sandeen, cem
On Fri, Aug 09, 2024 at 09:23:26AM -0700, Darrick J. Wong wrote:
> On Fri, Aug 09, 2024 at 11:15:11AM -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.
> > v4: add logic to cover error case when ip is not attained.
> > ---
> > db/iunlink.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/db/iunlink.c b/db/iunlink.c
> > index d87562e3..98d1effc 100644
> > --- a/db/iunlink.c
> > +++ b/db/iunlink.c
> > @@ -49,8 +49,12 @@ get_next_unlinked(
> >
> > ino = XFS_AGINO_TO_INO(mp, agno, agino);
> > error = -libxfs_iget(mp, NULL, ino, 0, &ip);
> > - if (error)
> > - goto bad;
> > + if (error) {
> > + if (ip)
> > + goto bad_rele;
>
> When does libxfs_iget return nonzero and a non-NULL ip? Wouldn't 'goto
> bad' suffice here?
Yes, and I thought that in v1. My mistake here. I'll send a v5.
-Bill
>
> --D
>
> > + else
> > + goto bad;
> > + }
> >
> > if (verbose) {
> > xfs_filblks_t blocks, rtblks = 0;
> > @@ -67,13 +71,16 @@ get_next_unlinked(
> >
> > error = -libxfs_imap_to_bp(mp, NULL, &ip->i_imap, &ino_bp);
> > if (error)
> > - goto bad;
> > + goto bad_rele;
> >
> > 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_rele:
> > + libxfs_irele(ip);
> > bad:
> > dbprintf(_("AG %u agino %u: %s\n"), agno, agino, strerror(error));
> > return NULLAGINO;
> > --
> > 2.46.0
> >
> >
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-08-09 17:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-09 16:15 [PATCH v4] xfs_db: release ip resource before returning from get_next_unlinked() Bill O'Donnell
2024-08-09 16:23 ` Darrick J. Wong
2024-08-09 17:03 ` Bill O'Donnell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox