Subject: Re: [PATCH 6/7] trailer: handle NULL value when parsing trailer-specific config

From: Patrick Steinhardt <[email-masked]>
Date: Thu, 7 Dec 2023 09:14:53 +0100

On Thu, Dec 07, 2023 at 02:11:32AM -0500, Jeff King wrote:
> When parsing the "key", "command", and "cmd" trailer config, we just
> make a copy of the value string.  If we see an implicit bool like:
> 
>   [trailer "foo"]
>   key
> 
> we'll segfault trying to copy a NULL pointer. We can fix this with the
> usual config_error_nonbool() check.
> 
> I split this out from the other vanilla cases, because at first glance
> it looks like a better fix here would be to move the NULL check out of
> the switch statement. But it would change the behavior of other keys
> like trailer.*.ifExists, where an implicit bool is interpreted as
> EXISTS_DEFAULT.
> 
> Signed-off-by: Jeff King <[email-masked]>
> ---
>  trailer.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/trailer.c b/trailer.c
> index b0e2ec224a..e4b08ed267 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -553,16 +553,22 @@ static int git_trailer_config(const char *conf_key, const char *value,
>  	case TRAILER_KEY:
>  		if (conf->key)
>  			warning(_("more than one %s"), conf_key);
> +		if (!value)
> +			return config_error_nonbool(conf_key);
>  		conf->key = xstrdup(value);
>  		break;
>  	case TRAILER_COMMAND:
>  		if (conf->command)
>  			warning(_("more than one %s"), conf_key);
> +		if (!value)
> +			return config_error_nonbool(conf_key);
>  		conf->command = xstrdup(value);
>  		break;
>  	case TRAILER_CMD:
>  		if (conf->cmd)
>  			warning(_("more than one %s"), conf_key);
> +		if (!value)
> +			return config_error_nonbool(conf_key);
>  		conf->cmd = xstrdup(value);
>  		break;
>  	case TRAILER_WHERE:

For the other cases we only generate warnings for unknown values, but
return successfully. Should we do the same here?

Patrick