Re: [PATCH v3 8/9] config API: add "string" version of *_value_multi(), fix segfaults

[Glen Choo <chooglen@google.com>]

Ævar Arnfjörð Bjarmason         <avarab@gmail.com> 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.