Re: [PATCH] config.c: NULL check when reading protected config

[Taylor Blau <me@ttaylorr.com>]

On Tue, Jul 26, 2022 at 05:09:32PM +0000, Glen Choo via GitGitGadget wrote:
> From: Glen Choo <chooglen@google.com>
>
> In read_protected_config(), check whether each file name is NULL before
> attempting to read it. This mirrors do_git_config_sequence() (which
> read_protected_config() is modelled after).

s/modelled/modeled

> Without these NULL checks,
>
>  make SANITIZE=address test T=t0410*.sh

I'm glad that t0410 was catching this for us already, though it is too
bad we didn't see it outside of the ASan builds, or I think we could
have potentially caught this earlier.

Either way, I think the test coverage here is sufficient, so what you
wrote makes sense.

> diff --git a/config.c b/config.c
> index 015bec360f5..b0ba7f439a4 100644
> --- a/config.c
> +++ b/config.c
> @@ -2645,9 +2645,12 @@ static void read_protected_config(void)
>  	system_config = git_system_config();
>  	git_global_config(&user_config, &xdg_config);
>
> -	git_configset_add_file(&protected_config, system_config);
> -	git_configset_add_file(&protected_config, xdg_config);
> -	git_configset_add_file(&protected_config, user_config);
> +	if (system_config)
> +		git_configset_add_file(&protected_config, system_config);
> +	if (xdg_config)
> +		git_configset_add_file(&protected_config, xdg_config);
> +	if (user_config)
> +		git_configset_add_file(&protected_config, user_config);
>  	git_configset_add_parameters(&protected_config);

I wonder: should it become a BUG() to call git_configset_add_file() with
a NULL filename? That would have elevated the test failure outside of
just the ASAn builds, I'd think.

There's certainty a risk of being too defensive, but elevating this
error beyond just the ASan builds indicates that this would be an
appropriate layer of defense IMHO.

Thanks,
Taylor