Re: [PATCH v2 08/22] config: fix leaking comment character config

[Patrick Steinhardt <ps@pks.im>]

On Thu, Aug 08, 2024 at 10:12:26AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > diff --git a/config.c b/config.c
> > index 6421894614..cb78b652ee 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -1596,7 +1596,9 @@ static int git_default_core_config(const char *var, const char *value,
> >  		else if (value[0]) {
> >  			if (strchr(value, '\n'))
> >  				return error(_("%s cannot contain newline"), var);
> > -			comment_line_str = xstrdup(value);
> > +			free(comment_line_str_allocated);
> > +			comment_line_str = comment_line_str_allocated =
> > +				xstrdup(value);
> 
> If you are to follow the _to_free pattern, you do not have to
> allocate here, no?  We borrow the value in the configset and point
> at it via comment_line_str, and clear comment_line_str_to_free
> because there is nothing to free now.  I.e.
> 
> 			comment_line_str = value;
> 			FREE_AND_NULL(comment_line_str_allocated);

Only if it is guaranteed that the configuration will never be re-read,
which would end up discarding memory owned by the old string. Which
should be the case already, but to the best of my knowledge we do not
document the expected lifetime of config strings anywhere.

> I still think the approach taken by the previous iteration was
> simpler and much less error prone, though.

I personally prefer this iteration. I feel that it is way more
discoverable to have an explicit indicator that something needs to be
freed, which the `_allocated` suffix brings us. With the old version,
the caller needs to become aware that the constant string may sometimes
need to be freed, and that sometimes is figured out by comparing to a
magic variable, which feels worse to me.

Ultimately, both solutions are okay-ish, but I don't consider either of
them to be great. As mentioned elsewhere, I think the best solution
would be to adapt the `struct strbuf` interface to have an initializer
like `STRBUF_INIT_CONST("foobar")` that allows us to initialize it with
a string constant. There wouldn't be any need to have two variables
anymore, and the `strbuf` API would handle the lifecycle of its contents
for us. In any case, I'd say this is a #leftoverbit and is better done
in a subsequent patch series.

I don't really think it makes sense to reroll this version to swap out
the patch for the first version again, but am happy to adapt if you
prefer that.

Thanks!

Patrick