Re: [PATCH 1/7] config: handle NULL value when parsing non-bools

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 7461 bytes --]

On Thu, Dec 07, 2023 at 02:11:14AM -0500, Jeff King wrote:
> When the config parser sees an "implicit" bool like:
> 
>   [core]
>   someVariable
> 
> it passes NULL to the config callback. Any callback code which expects a
> string must check for NULL. This usually happens via helpers like
> git_config_string(), etc, but some custom code forgets to do so and will
> segfault.
> 
> These are all fairly vanilla cases where the solution is just the usual
> pattern of:
> 
>   if (!value)
>         return config_error_nonbool(var);
> 
> though note that in a few cases we have to split initializers like:
> 
>   int some_var = initializer();
> 
> into:
> 
>   int some_var;
>   if (!value)
>         return config_error_nonbool(var);
>   some_var = initializer();
> 
> There are still some broken instances after this patch, which I'll
> address on their own in individual patches after this one.
> 
> Reported-by: Carlos Andrés Ramírez Cataño <antaigroupltda@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/blame.c        |  2 ++
>  builtin/checkout.c     |  2 ++
>  builtin/clone.c        |  2 ++
>  builtin/log.c          |  5 ++++-
>  builtin/pack-objects.c |  6 +++++-
>  compat/mingw.c         |  2 ++
>  config.c               |  8 ++++++++
>  diff.c                 | 19 ++++++++++++++++---
>  mailinfo.c             |  2 ++
>  notes-utils.c          |  2 ++
>  trailer.c              |  2 ++
>  11 files changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 9c987d6567..2433b7da5c 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -748,6 +748,8 @@ static int git_blame_config(const char *var, const char *value,
>  	}
>  
>  	if (!strcmp(var, "blame.coloring")) {
> +		if (!value)
> +			return config_error_nonbool(var);

In the `else` statement where we fail to parse the value we only
generate a warning and return successfully regardless. Should we do the
same here?

>  		if (!strcmp(value, "repeatedLines")) {
>  			coloring_mode |= OUTPUT_COLOR_LINE;
>  		} else if (!strcmp(value, "highlightRecent")) {
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index f02434bc15..d5c784854f 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1202,6 +1202,8 @@ static int git_checkout_config(const char *var, const char *value,
>  	struct checkout_opts *opts = cb;
>  
>  	if (!strcmp(var, "diff.ignoresubmodules")) {
> +		if (!value)
> +			return config_error_nonbool(var);
>  		handle_ignore_submodules_arg(&opts->diff_options, value);
>  		return 0;
>  	}

This one is fine, `handle_ignore_submodules_arg()` dies if it gets an
unknown value and `git_config()` will die when the callback function
returns an error. The same is true for many other cases you've
converted.

[snip]
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 89a8b5a976..62c540b4db 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3204,14 +3204,18 @@ static int git_pack_config(const char *k, const char *v,
>  		return 0;
>  	}
>  	if (!strcmp(k, "uploadpack.blobpackfileuri")) {
> -		struct configured_exclusion *ex = xmalloc(sizeof(*ex));
> +		struct configured_exclusion *ex;
>  		const char *oid_end, *pack_end;
>  		/*
>  		 * Stores the pack hash. This is not a true object ID, but is
>  		 * of the same form.
>  		 */
>  		struct object_id pack_hash;
>  
> +		if (!v)
> +			return config_error_nonbool(k);
> +
> +		ex = xmalloc(sizeof(*ex));
>  		if (parse_oid_hex(v, &ex->e.oid, &oid_end) ||
>  		    *oid_end != ' ' ||
>  		    parse_oid_hex(oid_end + 1, &pack_hash, &pack_end) ||

This isn't part of the diff and not a new issue, but why don't we
`return 0` when parsing this config correctly? We fall through to
`git_default_config()` even if we've successfully parsed the config key,
which seems like a bug to me.

Anyway, this case looks fine.

[snip]
> diff --git a/config.c b/config.c
> index b330c7adb4..18085c7e38 100644
> --- a/config.c
> +++ b/config.c
> @@ -1386,6 +1386,8 @@ static int git_default_core_config(const char *var, const char *value,
>  		return 0;
>  	}
>  	if (!strcmp(var, "core.checkstat")) {
> +		if (!value)
> +			return config_error_nonbool(var);
>  		if (!strcasecmp(value, "default"))
>  			check_stat = 1;
>  		else if (!strcasecmp(value, "minimal"))

We would ignore `true` here, so should we ignore implicit `true`, as
well?

> @@ -1547,11 +1549,15 @@ static int git_default_core_config(const char *var, const char *value,
>  	}
>  
>  	if (!strcmp(var, "core.checkroundtripencoding")) {
> +		if (!value)
> +			return config_error_nonbool(var);
>  		check_roundtrip_encoding = xstrdup(value);
>  		return 0;
>  	}
>  
>  	if (!strcmp(var, "core.notesref")) {
> +		if (!value)
> +			return config_error_nonbool(var);
>  		notes_ref_name = xstrdup(value);
>  		return 0;
>  	}

I wonder the same here. We might as well use `xstrdup_or_null()`, but it
feels like the right thing to do to convert these to actual errors.

> @@ -426,10 +429,15 @@ int git_diff_ui_config(const char *var, const char *value,
>  	if (!strcmp(var, "diff.orderfile"))
>  		return git_config_pathname(&diff_order_file_cfg, var, value);
>  
> -	if (!strcmp(var, "diff.ignoresubmodules"))
> +	if (!strcmp(var, "diff.ignoresubmodules")) {
> +		if (!value)
> +			return config_error_nonbool(var);
>  		handle_ignore_submodules_arg(&default_diff_options, value);
> +	}
>  
>  	if (!strcmp(var, "diff.submodule")) {
> +		if (!value)
> +			return config_error_nonbool(var);
>  		if (parse_submodule_params(&default_diff_options, value))
>  			warning(_("Unknown value for 'diff.submodule' config variable: '%s'"),
>  				value);

Should we generate a warning instead according to the preexisting code
for "diff.submodule"?

> @@ -490,6 +501,8 @@ int git_diff_basic_config(const char *var, const char *value,
>  
>  	if (!strcmp(var, "diff.dirstat")) {
>  		struct strbuf errmsg = STRBUF_INIT;
> +		if (!value)
> +			return config_error_nonbool(var);
>  		default_diff_options.dirstat_permille = diff_dirstat_permille_default;
>  		if (parse_dirstat_params(&default_diff_options, value, &errmsg))
>  			warning(_("Found errors in 'diff.dirstat' config variable:\n%s"),

Same here, should we generate a warning instead?

> diff --git a/notes-utils.c b/notes-utils.c
> index 97c031c26e..01f4f5b424 100644
> --- a/notes-utils.c
> +++ b/notes-utils.c
> @@ -112,6 +112,8 @@ static int notes_rewrite_config(const char *k, const char *v,
>  		}
>  		return 0;
>  	} else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
> +		if (!v)
> +			return config_error_nonbool(k);
>  		/* note that a refs/ prefix is implied in the
>  		 * underlying for_each_glob_ref */
>  		if (starts_with(v, "refs/notes/"))

Here, as well.

> diff --git a/trailer.c b/trailer.c
> index b6de5d9cb2..b0e2ec224a 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -507,6 +507,8 @@ static int git_trailer_default_config(const char *conf_key, const char *value,
>  				warning(_("unknown value '%s' for key '%s'"),
>  					value, conf_key);
>  		} else if (!strcmp(trailer_item, "separators")) {
> +			if (!value)
> +				return config_error_nonbool(conf_key);
>  			separators = xstrdup(value);
>  		}
>  	}

And here.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]