| Message ID | 20240802222552.64389-1-bodonnel@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs_db: release ip resource before returning from get_next_unlinked() | expand |
On Fri, Aug 02, 2024 at 05:25:52PM -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> > --- > db/iunlink.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/db/iunlink.c b/db/iunlink.c > index d87562e3..3b2417c5 100644 > --- a/db/iunlink.c > +++ b/db/iunlink.c > @@ -72,6 +72,7 @@ get_next_unlinked( > 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); I think this needs to cover the error return for libxfs_imap_to_bp too, doesn't it? --D > > return ret; > bad: > -- > 2.45.2 >
On Fri, Aug 02, 2024 at 04:23:00PM -0700, Darrick J. Wong wrote: > On Fri, Aug 02, 2024 at 05:25:52PM -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> > > --- > > db/iunlink.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/db/iunlink.c b/db/iunlink.c > > index d87562e3..3b2417c5 100644 > > --- a/db/iunlink.c > > +++ b/db/iunlink.c > > @@ -72,6 +72,7 @@ get_next_unlinked( > > 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); > > I think this needs to cover the error return for libxfs_imap_to_bp too, > doesn't it? I considered that, but there are several places in the code where the error return doesn't release the resource. Not that that's correct, but the scans didn't flag them. For example, in bmap_inflate.c, bmapinflate_f() does not release the resource and scans didn't flag it. Thanks- Bill > > --D > > > > > return ret; > > bad: > > -- > > 2.45.2 > > >
On Mon, Aug 05, 2024 at 09:42:15AM -0500, Bill O'Donnell wrote: > On Fri, Aug 02, 2024 at 04:23:00PM -0700, Darrick J. Wong wrote: > > On Fri, Aug 02, 2024 at 05:25:52PM -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> > > > --- > > > db/iunlink.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/db/iunlink.c b/db/iunlink.c > > > index d87562e3..3b2417c5 100644 > > > --- a/db/iunlink.c > > > +++ b/db/iunlink.c > > > @@ -72,6 +72,7 @@ get_next_unlinked( > > > 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); > > > > I think this needs to cover the error return for libxfs_imap_to_bp too, > > doesn't it? > > I considered that, but there are several places in the code where the > error return doesn't release the resource. Not that that's correct, but the > scans didn't flag them. For example, in bmap_inflate.c, bmapinflate_f() > does not release the resource and scans didn't flag it. Looking at libxfs_iget(), it seems that for error cases, the resource is released within that function. -Bill > > Thanks- > Bill > > > > > > --D > > > > > > > > return ret; > > > bad: > > > -- > > > 2.45.2 > > > > > > >
On 8/5/24 9:42 AM, Bill O'Donnell wrote: > On Fri, Aug 02, 2024 at 04:23:00PM -0700, Darrick J. Wong wrote: >> On Fri, Aug 02, 2024 at 05:25:52PM -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> >>> --- >>> db/iunlink.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/db/iunlink.c b/db/iunlink.c >>> index d87562e3..3b2417c5 100644 >>> --- a/db/iunlink.c >>> +++ b/db/iunlink.c >>> @@ -72,6 +72,7 @@ get_next_unlinked( >>> 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); >> >> I think this needs to cover the error return for libxfs_imap_to_bp too, >> doesn't it? > > I considered that, but there are several places in the code where the > error return doesn't release the resource. Not that that's correct, but the > scans didn't flag them. For example, in bmap_inflate.c, bmapinflate_f() > does not release the resource and scans didn't flag it. Upstream coverity scan does flag it, CID 1554242 (that CID actually covers both instances of the leak). -Eric > Thanks- > Bill > > >> >> --D >> >>> >>> return ret; >>> bad: >>> -- >>> 2.45.2 >>> >> > >
On Wed, Aug 07, 2024 at 12:07:37PM -0500, Eric Sandeen wrote: > On 8/5/24 9:42 AM, Bill O'Donnell wrote: > > On Fri, Aug 02, 2024 at 04:23:00PM -0700, Darrick J. Wong wrote: > >> On Fri, Aug 02, 2024 at 05:25:52PM -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> > >>> --- > >>> db/iunlink.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/db/iunlink.c b/db/iunlink.c > >>> index d87562e3..3b2417c5 100644 > >>> --- a/db/iunlink.c > >>> +++ b/db/iunlink.c > >>> @@ -72,6 +72,7 @@ get_next_unlinked( > >>> 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); > >> > >> I think this needs to cover the error return for libxfs_imap_to_bp too, > >> doesn't it? > > > > I considered that, but there are several places in the code where the > > error return doesn't release the resource. Not that that's correct, but the > > scans didn't flag them. For example, in bmap_inflate.c, bmapinflate_f() > > does not release the resource and scans didn't flag it. > > Upstream coverity scan does flag it, CID 1554242 (that CID actually covers > both instances of the leak). Ah, thanks. I'll send a v2. -Bill > >>> > >>> return ret; > >>> bad: > >>> -- > >>> 2.45.2 > >>> > >> > > > > >
diff --git a/db/iunlink.c b/db/iunlink.c index d87562e3..3b2417c5 100644 --- a/db/iunlink.c +++ b/db/iunlink.c @@ -72,6 +72,7 @@ get_next_unlinked( 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> --- db/iunlink.c | 1 + 1 file changed, 1 insertion(+)