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