* Re: [xfsprogs] mkfs: fixup memory leak on exit
2026-10-06 14:48 ` Darrick J. Wong
@ 2026-10-06 15:20 ` Wilfred Mallawa
2026-10-08 7:17 ` Christoph Hellwig
2026-10-08 11:20 ` Carlos Maiolino
2 siblings, 0 replies; 10+ messages in thread
From: Wilfred Mallawa @ 2026-10-06 15:20 UTC (permalink / raw)
To: djwong@kernel.org; +Cc: linux-xfs@vger.kernel.org, cem@kernel.org
On Tue, 2026-10-06 at 07:48 -0700, Darrick J. Wong wrote:
> On Tue, Oct 06, 2026 at 01:52:18PM +0000, Wilfred Mallawa wrote:
> > On Tue, 2026-10-06 at 12:51 +0200, Carlos Maiolino wrote:
> > > > mkfs/xfs_mkfs.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > > > index 334367b8..fec43987 100644
> > > > --- a/mkfs/xfs_mkfs.c
> > > > +++ b/mkfs/xfs_mkfs.c
> > > > @@ -6503,5 +6503,8 @@ main(
> > > > exit(1);
> > > >
> > > > libxfs_destroy(&xi);
> > > > + free(cli.xi->data.name);
> > > > + free(cli.xi->log.name);
> > > > + free(cli.xi->rt.name);
> > > > return 0;
> > > > }
> > >
> > > Hm, wouldn't perhaps be better to hook this into libxfs_destroy?
> > > Although this would likely need some other extra surgery on
> > > xfs_copy.c.
> > >
> > > It just sounds weird to me to free the memory here 'after' we
> > > already
> > > called
> > > libxfs_destroy() here.
> >
> > Ah yeah.... I back-ported this off the xraid tree, where we have a
> > bit
> > more cleanup after libxfs_destroy() and I think it makes sense to
> > do
> > there but maybe not here...
> >
> > > IIUC: cli.xi == &xi in this place, so freeing up
> > > memory after destroying the struct using it sounds like a time
> > > bomb.
> >
> > I can plumb this into libxfs_destroy() but as you mentioned, it
> > will
> > need changes in xfs_copy, xfs_db and xfs_repair because they track
> > the
> > argv pointers, but we can just strdup(). Does that seem okay?
>
> I don't think this is better. libxfs' init functions do not create
> or
> assign the libxfs_init.{data,log,rt}.name fields, they merely reuse
> whatever pointer the calling program set. Only the caller knows
> where
> that pointed to memory came from (stack, optarg, dynamic allocation)
> so
> only the caller knows how to drop the pointer.
>
> The original patch is more appropriate IMO but seeing as we're about
> to
> return from main and terminate the process, why do we care?
I agree, I don't think there's any actual harm. Only that addrsan was
complaining when running mkfs.xfs (see below) and it was just annoying
to filter through a bunch of actual leaks that needed to be fixed.
```
==100203==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 9 byte(s) in 1 object(s) allocated from:
#0 0x7f6207ae8ac4 in strdup (/lib64/libasan.so.8+0xe8ac4) (BuildId:
5395ec74f54d9ec7bf97c06583dd39a96c230822)
#1 0x0000004175ce in getstr /home/twilfred/wdc/xfs/xfsprogs-
dev/mkfs/xfs_mkfs.c:1767
SUMMARY: AddressSanitizer: 9 byte(s) leaked in 1 allocation(s).
```
> And if we
> do, shouldn't mkfs also free the other getstr() strings attached to
> the
> cli struct? And shouldn't the exit(1) paths do likewise?
> (To be clear, my answers are "I don't", "no", and "no" but I could be
> convinced.)
>
Okay, fair enough :)
Wilfred
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [xfsprogs] mkfs: fixup memory leak on exit
2026-10-06 14:48 ` Darrick J. Wong
2026-10-06 15:20 ` Wilfred Mallawa
@ 2026-10-08 7:17 ` Christoph Hellwig
2026-10-08 11:20 ` Carlos Maiolino
2 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2026-10-08 7:17 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Wilfred Mallawa, cem@kernel.org, linux-xfs@vger.kernel.org
On Tue, Oct 06, 2026 at 07:48:51AM -0700, Darrick J. Wong wrote:
>
> I don't think this is better. libxfs' init functions do not create or
> assign the libxfs_init.{data,log,rt}.name fields, they merely reuse
> whatever pointer the calling program set. Only the caller knows where
> that pointed to memory came from (stack, optarg, dynamic allocation) so
> only the caller knows how to drop the pointer.
Exactly.
> The original patch is more appropriate IMO but seeing as we're about to
> return from main and terminate the process, why do we care? And if we
> do, shouldn't mkfs also free the other getstr() strings attached to the
> cli struct?
We probably want that, as having a clean xfstests run with addrsan is
actually really nice to have, as without that the addrsan support is
only if limited use.
> And shouldn't the exit(1) paths do likewise?
Maybe. Although it doesn't really matter all that much.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [xfsprogs] mkfs: fixup memory leak on exit
2026-10-06 14:48 ` Darrick J. Wong
2026-10-06 15:20 ` Wilfred Mallawa
2026-10-08 7:17 ` Christoph Hellwig
@ 2026-10-08 11:20 ` Carlos Maiolino
2026-10-11 1:32 ` Wilfred Mallawa
2 siblings, 1 reply; 10+ messages in thread
From: Carlos Maiolino @ 2026-10-08 11:20 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Wilfred Mallawa, linux-xfs@vger.kernel.org
On Tue, Oct 06, 2026 at 07:48:51AM -0700, Darrick J. Wong wrote:
> On Tue, Oct 06, 2026 at 01:52:18PM +0000, Wilfred Mallawa wrote:
> > On Tue, 2026-10-06 at 12:51 +0200, Carlos Maiolino wrote:
> > > > mkfs/xfs_mkfs.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > > > index 334367b8..fec43987 100644
> > > > --- a/mkfs/xfs_mkfs.c
> > > > +++ b/mkfs/xfs_mkfs.c
> > > > @@ -6503,5 +6503,8 @@ main(
> > > > exit(1);
> > > >
> > > > libxfs_destroy(&xi);
> > > > + free(cli.xi->data.name);
> > > > + free(cli.xi->log.name);
> > > > + free(cli.xi->rt.name);
> > > > return 0;
> > > > }
> > >
> > > Hm, wouldn't perhaps be better to hook this into libxfs_destroy?
> > > Although this would likely need some other extra surgery on
> > > xfs_copy.c.
> > >
> > > It just sounds weird to me to free the memory here 'after' we already
> > > called
> > > libxfs_destroy() here.
> >
> > Ah yeah.... I back-ported this off the xraid tree, where we have a bit
> > more cleanup after libxfs_destroy() and I think it makes sense to do
> > there but maybe not here...
> >
> > > IIUC: cli.xi == &xi in this place, so freeing up
> > > memory after destroying the struct using it sounds like a time bomb.
> >
> > I can plumb this into libxfs_destroy() but as you mentioned, it will
> > need changes in xfs_copy, xfs_db and xfs_repair because they track the
> > argv pointers, but we can just strdup(). Does that seem okay?
>
> I don't think this is better. libxfs' init functions do not create or
> assign the libxfs_init.{data,log,rt}.name fields, they merely reuse
> whatever pointer the calling program set. Only the caller knows where
> that pointed to memory came from (stack, optarg, dynamic allocation) so
> only the caller knows how to drop the pointer.
Yeah, that's mostly why I thought we'd need to re-wire a bunch of stuff
which is probably not important.
>
> The original patch is more appropriate IMO but seeing as we're about to
> return from main and terminate the process, why do we care? And if we
> do, shouldn't mkfs also free the other getstr() strings attached to the
> cli struct? And shouldn't the exit(1) paths do likewise?
>
> (To be clear, my answers are "I don't", "no", and "no" but I could be
> convinced.)
Leaving them as-is is fine for now, might not be fine in the future, I'd
say if we are leaking them, we should fix it, even if it's falling to
the end of the program.
>
> --D
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [xfsprogs] mkfs: fixup memory leak on exit
2026-10-08 11:20 ` Carlos Maiolino
@ 2026-10-11 1:32 ` Wilfred Mallawa
2026-10-11 7:09 ` Carlos Maiolino
0 siblings, 1 reply; 10+ messages in thread
From: Wilfred Mallawa @ 2026-10-11 1:32 UTC (permalink / raw)
To: djwong@kernel.org, cem@kernel.org; +Cc: linux-xfs@vger.kernel.org
[snip]
> >
> > The original patch is more appropriate IMO but seeing as we're
> > about to
> > return from main and terminate the process, why do we care? And if
> > we
> > do, shouldn't mkfs also free the other getstr() strings attached to
> > the
> > cli struct? And shouldn't the exit(1) paths do likewise?
> >
> > (To be clear, my answers are "I don't", "no", and "no" but I could
> > be
> > convinced.)
>
> Leaving them as-is is fine for now, might not be fine in the future,
> I'd
> say if we are leaking them, we should fix it, even if it's falling to
> the end of the program.
>
Just to clarify, should I can spin up a V2 that frees all of the
getstr() allocations in xfs_mkfs? or shall we drop this for now?
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [xfsprogs] mkfs: fixup memory leak on exit
2026-10-11 1:32 ` Wilfred Mallawa
@ 2026-10-11 7:09 ` Carlos Maiolino
2026-10-12 0:43 ` Wilfred Mallawa
0 siblings, 1 reply; 10+ messages in thread
From: Carlos Maiolino @ 2026-10-11 7:09 UTC (permalink / raw)
To: Wilfred Mallawa; +Cc: djwong@kernel.org, linux-xfs@vger.kernel.org
On Sun, Oct 11, 2026 at 01:32:58AM +0000, Wilfred Mallawa wrote:
> [snip]
>
> > >
> > > The original patch is more appropriate IMO but seeing as we're
> > > about to
> > > return from main and terminate the process, why do we care? And if
> > > we
> > > do, shouldn't mkfs also free the other getstr() strings attached to
> > > the
> > > cli struct? And shouldn't the exit(1) paths do likewise?
> > >
> > > (To be clear, my answers are "I don't", "no", and "no" but I could
> > > be
> > > convinced.)
> >
> > Leaving them as-is is fine for now, might not be fine in the future,
> > I'd
> > say if we are leaking them, we should fix it, even if it's falling to
> > the end of the program.
> >
>
> Just to clarify, should I can spin up a V2 that frees all of the
> getstr() allocations in xfs_mkfs? or shall we drop this for now?
I'm happy getting rid of leaks even if they are just before we exit().
Final decision is from the maintainer thought. Andrey has been OOO past
few days, so perhaps wait for him to come back and decide?!
Cheers.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [xfsprogs] mkfs: fixup memory leak on exit
2026-10-11 7:09 ` Carlos Maiolino
@ 2026-10-12 0:43 ` Wilfred Mallawa
0 siblings, 0 replies; 10+ messages in thread
From: Wilfred Mallawa @ 2026-10-12 0:43 UTC (permalink / raw)
To: cem@kernel.org; +Cc: djwong@kernel.org, linux-xfs@vger.kernel.org
On Sun, 2026-10-11 at 09:09 +0200, Carlos Maiolino wrote:
> > >
> >
> > Just to clarify, should I can spin up a V2 that frees all of the
> > getstr() allocations in xfs_mkfs? or shall we drop this for now?
>
> I'm happy getting rid of leaks even if they are just before we
> exit().
> Final decision is from the maintainer thought. Andrey has been OOO
> past
> few days, so perhaps wait for him to come back and decide?!
>
> Cheers.
Okay, sounds good!
^ permalink raw reply [flat|nested] 10+ messages in thread