| Message ID | 20240807193801.248101-3-bodonnel@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v3] xfs_db: release ip resource before returning from get_next_unlinked() | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Aug 07, 2024 at 02:38:03PM -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. > --- > db/iunlink.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/db/iunlink.c b/db/iunlink.c > index d87562e3..57e51140 100644 > --- a/db/iunlink.c > +++ b/db/iunlink.c > @@ -66,15 +66,18 @@ get_next_unlinked( > } > > error = -libxfs_imap_to_bp(mp, NULL, &ip->i_imap, &ino_bp); > - if (error) > + if (error) { > + libxfs_buf_relse(ino_bp); Sorry, I think I've led you astray -- it's not necessary to libxfs_buf_relse in any of the bailouts. --D > 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: > + libxfs_irele(ip); > dbprintf(_("AG %u agino %u: %s\n"), agno, agino, strerror(error)); > return NULLAGINO; > } > -- > 2.45.2 > >
On 8/8/24 1:28 PM, Darrick J. Wong wrote: > On Wed, Aug 07, 2024 at 02:38:03PM -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. >> --- >> db/iunlink.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/db/iunlink.c b/db/iunlink.c >> index d87562e3..57e51140 100644 >> --- a/db/iunlink.c >> +++ b/db/iunlink.c >> @@ -66,15 +66,18 @@ get_next_unlinked( >> } >> >> error = -libxfs_imap_to_bp(mp, NULL, &ip->i_imap, &ino_bp); >> - if (error) >> + if (error) { >> + libxfs_buf_relse(ino_bp); > > Sorry, I think I've led you astray -- it's not necessary to > libxfs_buf_relse in any of the bailouts. > > --D > >> 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: >> + libxfs_irele(ip); And this addition results in a libxfs_irele of an ip() which failed iget() via the first goto bad, so you're releasing a thing which was never obtained, which doesn't make sense. There are 2 relevant actions here. The libxfs_iget, and the libxfs_imap_to_bp. Only after libxfs_iget(ip) /succeeds/ does it need a libxfs_irele(ip), on either error paths or normal exit. The fact that it does neither leads to the two leaks noted in CID 1554242. libxfs_imap_to_bp needs a corresponding libxfs_buf_relse() (thanks for clarifying djwong) but that libxfs_buf_relse() is already present if libxfs_imap_to_bp succeeds. It's not needed if it fails, because there's nothing to release. When Darrick said > I think this needs to cover the error return for libxfs_imap_to_bp too, > doesn't it? I think he meant that in the error case where libxfs_imap_to_bp fails, libxfs_irele is also needed. (In addition to being needed on a normal return.) -Eric
On Thu, Aug 08, 2024 at 02:00:22PM -0500, Eric Sandeen wrote: > On 8/8/24 1:28 PM, Darrick J. Wong wrote: > > On Wed, Aug 07, 2024 at 02:38:03PM -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. > >> --- > >> db/iunlink.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/db/iunlink.c b/db/iunlink.c > >> index d87562e3..57e51140 100644 > >> --- a/db/iunlink.c > >> +++ b/db/iunlink.c > >> @@ -66,15 +66,18 @@ get_next_unlinked( > >> } > >> > >> error = -libxfs_imap_to_bp(mp, NULL, &ip->i_imap, &ino_bp); > >> - if (error) > >> + if (error) { > >> + libxfs_buf_relse(ino_bp); > > > > Sorry, I think I've led you astray -- it's not necessary to > > libxfs_buf_relse in any of the bailouts. > > > > --D > > > >> 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: > >> + libxfs_irele(ip); > > And this addition results in a libxfs_irele of an ip() which failed iget() > via the first goto bad, so you're releasing a thing which was never obtained, > which doesn't make sense. > > > There are 2 relevant actions here. The libxfs_iget, and the libxfs_imap_to_bp. > Only after libxfs_iget(ip) /succeeds/ does it need a libxfs_irele(ip), on either > error paths or normal exit. The fact that it does neither leads to the two leaks > noted in CID 1554242. In libxfs_iget, -ENOMEM is returned when kmem_cache_zalloc() fails. For all other error cases in that function, kmem_cache_free() releases the memory that was presumably successfully allocated. I had wondered if we need to use libxfs_irele() at all in get_next_unlinked() (except for the success case)? > libxfs_imap_to_bp needs a corresponding libxfs_buf_relse() (thanks for clarifying > djwong) but that libxfs_buf_relse() is already present if libxfs_imap_to_bp > succeeds. It's not needed if it fails, because there's nothing to release. > > When Darrick said > > > I think this needs to cover the error return for libxfs_imap_to_bp too, > > doesn't it? > > I think he meant that in the error case where libxfs_imap_to_bp fails, libxfs_irele > is also needed. (In addition to being needed on a normal return.) > > -Eric >
On Thu, Aug 08, 2024 at 02:41:39PM -0500, Bill O'Donnell wrote: > On Thu, Aug 08, 2024 at 02:00:22PM -0500, Eric Sandeen wrote: > > On 8/8/24 1:28 PM, Darrick J. Wong wrote: > > > On Wed, Aug 07, 2024 at 02:38:03PM -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. > > >> --- > > >> db/iunlink.c | 7 +++++-- > > >> 1 file changed, 5 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/db/iunlink.c b/db/iunlink.c > > >> index d87562e3..57e51140 100644 > > >> --- a/db/iunlink.c > > >> +++ b/db/iunlink.c > > >> @@ -66,15 +66,18 @@ get_next_unlinked( > > >> } > > >> > > >> error = -libxfs_imap_to_bp(mp, NULL, &ip->i_imap, &ino_bp); > > >> - if (error) > > >> + if (error) { > > >> + libxfs_buf_relse(ino_bp); > > > > > > Sorry, I think I've led you astray -- it's not necessary to > > > libxfs_buf_relse in any of the bailouts. > > > > > > --D > > > > > >> 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: > > >> + libxfs_irele(ip); > > > > And this addition results in a libxfs_irele of an ip() which failed iget() > > via the first goto bad, so you're releasing a thing which was never obtained, > > which doesn't make sense. > > > > > > There are 2 relevant actions here. The libxfs_iget, and the libxfs_imap_to_bp. > > Only after libxfs_iget(ip) /succeeds/ does it need a libxfs_irele(ip), on either > > error paths or normal exit. The fact that it does neither leads to the two leaks > > noted in CID 1554242. > > In libxfs_iget, -ENOMEM is returned when kmem_cache_zalloc() fails. For all other > error cases in that function, kmem_cache_free() releases the memory that was presumably > successfully allocated. I had wondered if we need to use libxfs_irele() at all in > get_next_unlinked() (except for the success case)? So, if that's the case, I'm back to v1 of this patch. -Bill > > > > libxfs_imap_to_bp needs a corresponding libxfs_buf_relse() (thanks for clarifying > > djwong) but that libxfs_buf_relse() is already present if libxfs_imap_to_bp > > succeeds. It's not needed if it fails, because there's nothing to release. > > > > When Darrick said > > > > > I think this needs to cover the error return for libxfs_imap_to_bp too, > > > doesn't it? > > > > I think he meant that in the error case where libxfs_imap_to_bp fails, libxfs_irele > > is also needed. (In addition to being needed on a normal return.) > > > > -Eric > > > >
On 8/8/24 4:13 PM, Bill O'Donnell wrote: > On Thu, Aug 08, 2024 at 02:41:39PM -0500, Bill O'Donnell wrote: >> On Thu, Aug 08, 2024 at 02:00:22PM -0500, Eric Sandeen wrote: >>> On 8/8/24 1:28 PM, Darrick J. Wong wrote: >>>> On Wed, Aug 07, 2024 at 02:38:03PM -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. >>>>> --- >>>>> db/iunlink.c | 7 +++++-- >>>>> 1 file changed, 5 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/db/iunlink.c b/db/iunlink.c >>>>> index d87562e3..57e51140 100644 >>>>> --- a/db/iunlink.c >>>>> +++ b/db/iunlink.c >>>>> @@ -66,15 +66,18 @@ get_next_unlinked( >>>>> } >>>>> >>>>> error = -libxfs_imap_to_bp(mp, NULL, &ip->i_imap, &ino_bp); >>>>> - if (error) >>>>> + if (error) { >>>>> + libxfs_buf_relse(ino_bp); >>>> >>>> Sorry, I think I've led you astray -- it's not necessary to >>>> libxfs_buf_relse in any of the bailouts. >>>> >>>> --D >>>> >>>>> 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: >>>>> + libxfs_irele(ip); >>> >>> And this addition results in a libxfs_irele of an ip() which failed iget() >>> via the first goto bad, so you're releasing a thing which was never obtained, >>> which doesn't make sense. >>> >>> >>> There are 2 relevant actions here. The libxfs_iget, and the libxfs_imap_to_bp. >>> Only after libxfs_iget(ip) /succeeds/ does it need a libxfs_irele(ip), on either >>> error paths or normal exit. The fact that it does neither leads to the two leaks >>> noted in CID 1554242. >> >> In libxfs_iget, -ENOMEM is returned when kmem_cache_zalloc() fails. For all other >> error cases in that function, kmem_cache_free() releases the memory that was presumably >> successfully allocated. I had wondered if we need to use libxfs_irele() at all in >> get_next_unlinked() (except for the success case)? > > So, if that's the case, I'm back to v1 of this patch. > -Bill when libxfs_iget succeeds, it has obtained an inode. After that has happened, libxfs_irele needs to be called either in the normal return path, or any subsequent error path, to free that inode in this function. As Darrick pointed out in his first reply, V1 missed irele on the error path, so it was not sufficient. -Eric
diff --git a/db/iunlink.c b/db/iunlink.c index d87562e3..57e51140 100644 --- a/db/iunlink.c +++ b/db/iunlink.c @@ -66,15 +66,18 @@ get_next_unlinked( } error = -libxfs_imap_to_bp(mp, NULL, &ip->i_imap, &ino_bp); - if (error) + if (error) { + libxfs_buf_relse(ino_bp); 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: + libxfs_irele(ip); 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. --- db/iunlink.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)