git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Patrick Steinhardt <ps@pks.im>
Date
Dec 7, 2023, 08:14 UTC
Message-ID
<ZXF-8iNH0qaJSVl9@tanuki>
In-Reply-To
<20231207071114.GA1276005@coredump.intra.peff.net>
On Thu, Dec 07, 2023 at 02:11:14AM -0500, Jeff King wrote:
Show 56 quoted lines
> 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?

Show 16 quoted lines
>  		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]
Show 24 quoted lines
> 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]
Show 13 quoted lines
> 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?

Show 16 quoted lines
> @@ -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.

Show 17 quoted lines
> @@ -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"?

Show 9 quoted lines
> @@ -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?
Show 13 quoted lines
> 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.
Show 13 quoted lines
> 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
Previous: Jeff KingNext: Jeff King
Message 3 of 17 in “fix segfaults with implicit-bool config”
  1. 0/7 fix segfaults with implicit-bool configJeff King, Dec 7, 2023
  2. 1/7 config: handle NULL value when parsing non-boolsJeff King, Dec 7, 2023
  3. Patrick SteinhardtDec 7, 2023
  4. Jeff KingDec 12, 2023
  5. 2/7 setup: handle NULL value when parsing extensionsJeff King, Dec 7, 2023
  6. 3/7 trace2: handle NULL values in tr2_sysenv config callbackJeff King, Dec 7, 2023
  7. 4/7 help: handle NULL value for alias.* configJeff King, Dec 7, 2023
  8. 5/7 submodule: handle NULL value when parsing submodule.*.branchJeff King, Dec 7, 2023
  9. Patrick SteinhardtDec 7, 2023
  10. Jeff KingDec 12, 2023
  11. 6/7 trailer: handle NULL value when parsing trailer-specific configJeff King, Dec 7, 2023
  12. Patrick SteinhardtDec 7, 2023
  13. 7/7 fsck: handle NULL value when parsing message configJeff King, Dec 7, 2023
  14. Patrick SteinhardtDec 7, 2023
  15. Patrick SteinhardtDec 7, 2023
  16. Jeff KingDec 12, 2023
  17. Patrick SteinhardtDec 12, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.