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

[Carlos Maiolino <cem@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
>