On Sat, May 25, 2024 at 12:33:47AM -0400, Jeff King wrote:
> On Fri, May 24, 2024 at 12:04:12PM +0200, Patrick Steinhardt wrote:
>
> > diff --git a/alias.c b/alias.c
> > index 269892c356..4daafd9bda 100644
> > --- a/alias.c
> > +++ b/alias.c
> > @@ -21,9 +21,11 @@ static int config_alias_cb(const char *key, const char *value,
> > return 0;
> >
> > if (data->alias) {
> > - if (!strcasecmp(p, data->alias))
> > + if (!strcasecmp(p, data->alias)) {
> > + FREE_AND_NULL(data->v);
> > return git_config_string(&data->v,
> > key, value);
> > + }
> > } else if (data->list) {
> > string_list_append(data->list, p);
> > }
>
> IMHO this should be done automatically by git_config_string(). The
> current design is an accident waiting to happen, and in the long run
> every call is going to need this FREE_AND_NULL(). By doing it in the
> function the calling code is shorter, and there's no way we'll forget.
In fact, I had this in my first iteration. But I didn't feel comfortable
with it exactly due to the reasons you mention below, where often times
we assign string constants as default values. This requires us to be
extremely careful, also because we do not yet have `-Wwrite-strings`
enabled as you mention.
> I posted a series along those lines a month or so ago:
>
> https://lore.kernel.org/git/20240407005656.GA436890@xxxxxxxxxxxxxxxxxxxxxxx/
>
> The catch is that you can't do this:
>
> const char *foo = "bar";
> git_config_string(&foo, ...);
>
> So I introduced a new function that took a non-const pointer with the
> new behavior, with the idea that we'd eventually migrate everything
> over. It looks like you may have already done that migration earlier in
> your series, since the move to "char *" in the previous patch was OK.
>
> Though as a side note, sadly:
>
> char *foo = "bar";
>
> does not produce an error or even a warning without -Wwrite-strings. I
> think in the long run we should enable that, but there's a little
> cleanup required to do so.
Indeed, I had the exact same observation. I've already got a patch
series that enables `-Wwrite-strings` and that adapts our codebase to
compile cleanly with it. I'll send that series once this one here has
landed.
So my proposal would be to leave this patch as-is for the time being,
but revisit it once both patch series have landed. Does that work for
you?
> The main reason I didn't follow up on that earlier series is that
> there was some discussion about maybe moving this stuff over to
> strbufs (after teaching it to handle literal initializers). But if
> you've managed to remove all of the cases that needed that, I think
> just sticking with "char *" is fine.
I don't think I managed to hit every callsite yet that leaks memory. But
I think it shouldn't be too bad, and especiall if we follow up this
patch series with `FREE_AND_NULL()` on the out-parameter then this
should be fine.
> The other issue raised in that thread is that many of these config
> variables are also passed to parse-options, which treats them as const
> strings (and we get no compiler support because it goes through a void
> pointer). So they may leak if we overwrite them, or in the unusual
> case that we load config after parsing options, we may try to free a
> non-heap string. The one we discussed was log's signature_file, and it
> looks like you split that to use two variables, which works. Junio
> suggested an OPT_FILENAME_DUP() option, which I'm also OK with. The
> main challenge to me is being sure we found all such spots (and not
> accidentally introducing new ones). But I don't have a good solution
> there.
Yup, I remember having some issues with `OPT_FILENAME()`, but to the
best of my knowledge I've fixed all of them.
> > @@ -1566,7 +1569,7 @@ static int git_default_core_config(const char
> > *var, const char *value,
> >
> > if (!strcmp(var, "core.checkroundtripencoding")) {
> > FREE_AND_NULL(check_roundtrip_encoding); - return
> > git_config_string((const char **) &check_roundtrip_encoding,
> > var, value); + return
> > git_config_string(&check_roundtrip_encoding, var, value); }
>
> This should have lost its cast in the previous commit, no? Applying up
> to patch 12 and building with DEVELOPER=1 gets a warning.
Ah, good catch. Will fix.
Patrick
Attachment:
signature.asc
Description: PGP signature
