| Message ID | 20240809161509.357133-3-bodonnel@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v4] xfs_db: release ip resource before returning from get_next_unlinked() | expand |
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 > >
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 > > > > >
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;
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(-)