Re: [PATCH 07/13] dir: free allocations on parse-error paths in read_one_dir()

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Patrick,

On Wed, 1 Jul 2026, Patrick Steinhardt wrote:

> On Wed, Jul 01, 2026 at 07:04:25AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/dir.c b/dir.c
> > index 32430090dc..23335b9f7a 100644
> > --- a/dir.c
> > +++ b/dir.c
> > @@ -3792,13 +3792,18 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
> >  		ALLOC_ARRAY(ud.untracked, ud.untracked_nr);
> >  
> >  	ud.dirs_alloc = ud.dirs_nr = decode_varint(&data);
> > -	if (data > end)
> > +	if (data > end) {
> > +		free(ud.untracked);
> >  		return -1;
> > +	}
> >  	ALLOC_ARRAY(ud.dirs, ud.dirs_nr);
> >  
> >  	eos = memchr(data, '\0', end - data);
> > -	if (!eos || eos == end)
> > +	if (!eos || eos == end) {
> > +		free(ud.untracked);
> > +		free(ud.dirs);
> >  		return -1;
> > +	}
> >  
> >  	*untracked_ = untracked = xmalloc(st_add3(sizeof(*untracked), eos - data, 1));
> >  	memcpy(untracked, &ud, sizeof(ud));
> 
> Hm. Here we assign ownership to the caller, but this still feels quite
> off to me as we also have two more early returns after this point that
> seem to leak memory. Do the callers make sure to always free the data?

Ownership transfers to the caller on the `xmalloc`/`memcpy` line: the
`memcpy` copies the `ud.untracked` and `ud.dirs` pointers into the freshly
xmalloc'd struct that becomes `*untracked_`. From there, any subsequent
failure in the caller reaches `free_untracked_cache()` and then
`free_untracked()`, which releases both arrays. So the two further early
returns are correct as-are.

I will fold that reasoning into the v2 commit message so a future
reader does not have to re-derive it.

Incidentally, and orthogonal to Coverity's leak report: on those same
failure paths, individual slots of `->dirs` and `->untracked` remain
uninitialised, so `free_untracked()` walks garbage pointers before it
ever reaches the two `free()` calls above. That is a separate
crash-on-cleanup bug and I would prefer to address it in a follow-up
rather than widen the scope of this series.

Ciao,
Johannes