Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> Fix numerous and mostly long-standing segfaults in consumers of
> the *_config_*value_multi() API. As discussed in the preceding commit
> an empty key in the config syntax yields a "NULL" string, which these
> users would give to strcmp() (or similar), resulting in segfaults.
>
> As this change shows, most users users of the *_config_*value_multi()
> API didn't really want such an an unsafe and low-level API, let's give
> them something with the safety of git_config_get_string() instead.
I think the low-level API argument makes sense. All of the other
*_get_*() functions perform some kind of validation, e.g.
config_parse_*() for non-string types and config_error_nonbool() for
strings. In effect, *_get_value_multi() was returning raw output from
the config parser without any concern for the caller.
> This fix is similar to what the *_string() functions and others
> acquired in[1] and [2]. Namely introducing and using a safer
> "*_get_string_multi()" variant of the low-level "_*value_multi()"
> function.
This suggests to me that we should really get rid of
*_get_value_multi(), since nobody outside of config.c should need it. I
don't think we'd ever end up in a situation where the caller wants the
raw strings from the config parser (unless we had a config
key which accepted values of different types? but that sounds like a
terrible mistake).
> There are now three remaining files using the low-level API:
>
> - Two cases in "builtin/submodule--helper.c", where it's used safely
> to see if any config exists.
> - One in "builtin/for-each-repo.c", which we'll convert in a
> subsequent commit.
> - The "t/helper/test-config.c" code added in [3].
As you noted, the only remaining non-test caller of the low-level API is
builtin/submodule--helper.c, which maybe we could safely convert anyway
and get rid of the API altogether. I'm okay with that being a leftover
bit, but maybe that's worth noting in the CL.
> The callback pattern being used here will make it easy to introduce
> e.g. a "multi" variant which coerces its values to "bool", "int",
> "path" etc.
I like that this is quite easily extensible, e.g.
> diff --git a/config.c b/config.c
> index 0b07045ed8c..9bd43189c02 100644
> --- a/config.c
> +++ b/config.c
> @@ -2437,6 +2437,25 @@ int git_configset_get_value_multi(struct config_set *cs, const char *key,
> return 0;
> }
>
> +static int check_multi_string(struct string_list_item *item, void *util)
> +{
> + return item->string ? 0 : config_error_nonbool(util);
> +}
> +
> +int git_configset_get_string_multi(struct config_set *cs, const char *key,
> + const struct string_list **dest)
> +{
> + int ret;
> +
> + if ((ret = git_configset_get_value_multi(cs, key, dest)))
> + return ret;
> + if ((ret = for_each_string_list((struct string_list *)*dest,
> + check_multi_string, (void *)key)))
> + return ret;
> +
> + return 0;
> +}
we could just use config_parse_<typename>() if we want to add, e.g.
*_get_bool_multi().
And as a reasonableness check, config_error_nonbool() is what we use to
validate the *_get_string() functions, so it makes sense to reuse it for
the string list version.
