[xfsprogs] mkfs: fixup memory leak on exit

[xfsprogs] mkfs: fixup memory leak on exit

[Wilfred Mallawa]

From: Wilfred Mallawa <wilfred.mallawa@wdc.com>

When running mkfs.xfs with addrsan enabled, the following is reported:

```
==27832==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 9 byte(s) in 1 object(s) allocated from:
    #0 0x7fe6db2e8ac4 in strdup (/lib64/libasan.so.8+0xe8ac4) (BuildId: 5395ec74f54d9ec7bf97c06583dd39a96c230822)
    #1 0x0000004175ce in getstr <blah>/mkfs/xfs_mkfs.c:1767

SUMMARY: AddressSanitizer: 9 byte(s) leaked in 1 allocation(s).
```

Thus, gracefully clean up these string allocation on exit to address the above.

Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
---
 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;
 }
-- 
2.55.0

Re: [xfsprogs] mkfs: fixup memory leak on exit

[Carlos Maiolino]

On Tue, Oct 06, 2026 at 12:12:33PM +0200, Wilfred Mallawa wrote:
> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> 
> When running mkfs.xfs with addrsan enabled, the following is reported:
> 
> ```
> ==27832==ERROR: LeakSanitizer: detected memory leaks
> 
> Direct leak of 9 byte(s) in 1 object(s) allocated from:
>     #0 0x7fe6db2e8ac4 in strdup (/lib64/libasan.so.8+0xe8ac4) (BuildId: 5395ec74f54d9ec7bf97c06583dd39a96c230822)
>     #1 0x0000004175ce in getstr <blah>/mkfs/xfs_mkfs.c:1767
> 
> SUMMARY: AddressSanitizer: 9 byte(s) leaked in 1 allocation(s).
> ```
> 
> Thus, gracefully clean up these string allocation on exit to address the above.
> 
> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> ---
>  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. IIUC: cli.xi == &xi in this place, so freeing up
memory after destroying the struct using it sounds like a time bomb.

Re: [xfsprogs] mkfs: fixup memory leak on exit

[Wilfred Mallawa]

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?

Thanks!

Re: [xfsprogs] mkfs: fixup memory leak on exit

[Darrick J. Wong]

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?  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.)

--D

Re: [xfsprogs] mkfs: fixup memory leak on exit

[Wilfred Mallawa]

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

Re: [xfsprogs] mkfs: fixup memory leak on exit

[Christoph Hellwig]

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.

Re: [xfsprogs] mkfs: fixup memory leak on exit

[Carlos Maiolino]

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
> 

Re: [xfsprogs] mkfs: fixup memory leak on exit

[Wilfred Mallawa]

[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?

> 

Re: [xfsprogs] mkfs: fixup memory leak on exit

[Carlos Maiolino]

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.

Re: [xfsprogs] mkfs: fixup memory leak on exit

[Wilfred Mallawa]

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!