Re: [PATCH v3 22/22] builtin/diff: free symmetric diff members

[Patrick Steinhardt <ps@pks.im>]

On Tue, Aug 13, 2024 at 09:25:41AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > We populate a `struct symdiff` in case the user has requested a
> > symmetric diff. Part of this is to populate a `skip` bitmap that
> > indicates which commits shall be ignored in the diff. But while this
> > bitmap is dynamically allocated, we never free it.
> >
> > Fix this by introducing and calling a new `symdiff_release()` function
> > that does this for us.
> 
> OK.
> 
> > +static void symdiff_release(struct symdiff *sdiff)
> > +{
> > +	if (!sdiff)
> > +		return;
> > +	bitmap_free(sdiff->skip);
> > +}
> 
> Hmph, wouldn't it be a BUG if any caller feeds a NULL pointer to it,
> though?  When symdiff was prepared but not used, sdiff->skip will be
> NULL but sdiff is never NULL even in such a case.

Good point. It does make sense for `_free()` functions to handle NULL
pointers, but doesn't quite for `_release()` ones.

> > @@ -398,7 +405,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
> >  	struct object_array_entry *blob[2];
> >  	int nongit = 0, no_index = 0;
> >  	int result;
> > -	struct symdiff sdiff;
> > +	struct symdiff sdiff = {0};
> 
> And symdiff_prepare() at least clears its .skip member to NULL, so
> this pre-initialization is probably not needed.  If we are preparing
> ourselves for future changes of the flow in this function (e.g.
> goto's that jump to the clean-up label from which symdiff_release()
> is always called, even when we did not call symdiff_prepare() on
> this thing), this is probably not sufficient to convey that
> intention (instead I'd use an explicit ".skip = NULL" to say "we
> might not even call _prepare() but this one is prepared to be passed
> to _release() even in such a case").
> 
> Given that there is no such goto exists, and that _prepare() always
> sets up the .skip member appropriately, I wonder if we are much
> better off leaving sdiff uninitialized at the declaration site here.
> If we add such a goto that bypasses _prepare() in the future, the
> compiler will notice that we are passing an uninitialized sdiff to
> _release(), no?

You'd hope it does, but it certainly depends on your compiler flags.
Various hardening flags for example implicitly initialize variables, and
I have a feeling that this also causes them to not emit any warnings
anymore. At least I only spot such warnings in CI.

In any case, yes, we can drop the initialization here.

Patrick