[PATCH] config: remove unneeded struct field

[PATCH] config: remove unneeded struct field

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As well as receiving the config key and value, config callbacks
also receive a "struct key_value_info" containing information about
the source of the key-value pair. Accessing the "path" field of
this struct from a callback passed to repo_config() results in a
use-after-free. This happens because repo_config() first populates a
configset by calling config_with_options() and then iterates over the
configset with the callback passed by the caller. When the configset
is constructed it takes a shallow copy of the "struct key_value_info"
for each config setting. This leads to the use-after-free as the
"path" member is freed before config_with_options() returns.

We could fix this by interning the "path" field as we do
for the "filename" field but the "path" field is not actually
needed. It is populated with a copy of the "path" field from "struct
config_source". That field was added in d14d42440d8 (config: disallow
relative include paths from blobs, 2014-02-19) to distinguish between
relative include directives in files and those in blobs. However,
since 1b8132d99d8 (i18n: config: unfold error messages marked for
translation, 2016-07-28) we can differentiate these by looking at the
"origin_type" field in "struct key_value_info". So let's remove the
"path" members from "struct config_source" and "struct key_value_info"
and instead use a combination of the "filename" and "origin_type"
fields to determine the absolute path of relative includes.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
I stumbled across this use-after-free while working on the deprecation
of core.commentChar=auto.
Base-Commit: a30f80fde927d70950b3b4d1820813480968fb0d
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fconfig-remove-kvi-path%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/a30f80fde...31724ce43
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/config-remove-kvi-path/v1

 config.c | 28 +++++++++++++---------------
 config.h |  2 --
 2 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/config.c b/config.c
index f55508bdc21..479ac7bf1e8 100644
--- a/config.c
+++ b/config.c
@@ -56,7 +56,6 @@ struct config_source {
 	} u;
 	enum config_origin_type origin_type;
 	const char *name;
-	const char *path;
 	enum config_error_action default_error_action;
 	int linenr;
 	int eof;
@@ -173,14 +172,14 @@ static int handle_path_include(const struct key_value_info *kvi,
 	if (!is_absolute_path(path)) {
 		char *slash;
 
-		if (!kvi || !kvi->path) {
+		if (!kvi || kvi->origin_type != CONFIG_ORIGIN_FILE) {
 			ret = error(_("relative config includes must come from files"));
 			goto cleanup;
 		}
 
-		slash = find_last_dir_sep(kvi->path);
+		slash = find_last_dir_sep(kvi->filename);
 		if (slash)
-			strbuf_add(&buf, kvi->path, slash - kvi->path + 1);
+			strbuf_add(&buf, kvi->filename, slash - kvi->filename + 1);
 		strbuf_addstr(&buf, path);
 		path = buf.buf;
 	}
@@ -224,11 +223,11 @@ static int prepare_include_condition_pattern(const struct key_value_info *kvi,
 	if (pat->buf[0] == '.' && is_dir_sep(pat->buf[1])) {
 		const char *slash;
 
-		if (!kvi || !kvi->path)
+		if (!kvi || kvi->origin_type != CONFIG_ORIGIN_FILE)
 			return error(_("relative config include "
 				       "conditionals must come from files"));
 
-		strbuf_realpath(&path, kvi->path, 1);
+		strbuf_realpath(&path, kvi->filename, 1);
 		slash = find_last_dir_sep(path.buf);
 		if (!slash)
 			BUG("how is this possible?");
@@ -633,7 +632,6 @@ void kvi_from_param(struct key_value_info *out)
 	out->linenr = -1;
 	out->origin_type = CONFIG_ORIGIN_CMDLINE;
 	out->scope = CONFIG_SCOPE_COMMAND;
-	out->path = NULL;
 }
 
 int git_config_parse_parameter(const char *text,
@@ -1036,7 +1034,6 @@ static void kvi_from_source(struct config_source *cs,
 	out->origin_type = cs->origin_type;
 	out->linenr = cs->linenr;
 	out->scope = scope;
-	out->path = cs->path;
 }
 
 static int git_parse_source(struct config_source *cs, config_fn_t fn,
@@ -1850,17 +1847,19 @@ static int do_config_from(struct config_source *top, config_fn_t fn,
 
 static int do_config_from_file(config_fn_t fn,
 			       const enum config_origin_type origin_type,
-			       const char *name, const char *path, FILE *f,
-			       void *data, enum config_scope scope,
+			       const char *name, FILE *f, void *data,
+			       enum config_scope scope,
 			       const struct config_options *opts)
 {
 	struct config_source top = CONFIG_SOURCE_INIT;
 	int ret;
 
+	if (origin_type == CONFIG_ORIGIN_FILE && (!name || !*name))
+		BUG("missing filename for CONFIG_ORIGIN_FILE");
+
 	top.u.file = f;
 	top.origin_type = origin_type;
 	top.name = name;
-	top.path = path;
 	top.default_error_action = CONFIG_ERROR_DIE;
 	top.do_fgetc = config_file_fgetc;
 	top.do_ungetc = config_file_ungetc;
@@ -1875,8 +1874,8 @@ static int do_config_from_file(config_fn_t fn,
 static int git_config_from_stdin(config_fn_t fn, void *data,
 				 enum config_scope scope)
 {
-	return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin,
-				   data, scope, NULL);
+	return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", stdin, data,
+				   scope, NULL);
 }
 
 int git_config_from_file_with_options(config_fn_t fn, const char *filename,
@@ -1891,7 +1890,7 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,
 	f = fopen_or_warn(filename, "r");
 	if (f) {
 		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,
-					  filename, f, data, scope, opts);
+					  f, data, scope, opts);
 		fclose(f);
 	}
 	return ret;
@@ -1916,7 +1915,6 @@ int git_config_from_mem(config_fn_t fn,
 	top.u.buf.pos = 0;
 	top.origin_type = origin_type;
 	top.name = name;
-	top.path = NULL;
 	top.default_error_action = CONFIG_ERROR_ERROR;
 	top.do_fgetc = config_buf_fgetc;
 	top.do_ungetc = config_buf_ungetc;
diff --git a/config.h b/config.h
index 29a02774837..cbb0f4fddcd 100644
--- a/config.h
+++ b/config.h
@@ -122,14 +122,12 @@ struct key_value_info {
 	int linenr;
 	enum config_origin_type origin_type;
 	enum config_scope scope;
-	const char *path;
 };
 #define KVI_INIT { \
 	.filename = NULL, \
 	.linenr = -1, \
 	.origin_type = CONFIG_ORIGIN_UNKNOWN, \
 	.scope = CONFIG_SCOPE_UNKNOWN, \
-	.path = NULL, \
 }
 
 /* Captures additional information that a config callback can use. */
-- 
2.49.0.897.gfad3eb7d210

Re: [PATCH] config: remove unneeded struct field

[Jeff King]

On Tue, Jul 15, 2025 at 03:00:56PM +0100, Phillip Wood wrote:

> We could fix this by interning the "path" field as we do
> for the "filename" field but the "path" field is not actually
> needed. It is populated with a copy of the "path" field from "struct
> config_source". That field was added in d14d42440d8 (config: disallow
> relative include paths from blobs, 2014-02-19) to distinguish between
> relative include directives in files and those in blobs. However,
> since 1b8132d99d8 (i18n: config: unfold error messages marked for
> translation, 2016-07-28) we can differentiate these by looking at the
> "origin_type" field in "struct key_value_info". So let's remove the
> "path" members from "struct config_source" and "struct key_value_info"
> and instead use a combination of the "filename" and "origin_type"
> fields to determine the absolute path of relative includes.

Nicely explained. The interesting bit of the patch is here:

> @@ -173,14 +172,14 @@ static int handle_path_include(const struct key_value_info *kvi,
>  	if (!is_absolute_path(path)) {
>  		char *slash;
>  
> -		if (!kvi || !kvi->path) {
> +		if (!kvi || kvi->origin_type != CONFIG_ORIGIN_FILE) {
>  			ret = error(_("relative config includes must come from files"));
>  			goto cleanup;
>  		}

...which is where we no longer need the separate variable to make the
path/filename distinction.

Just playing devil's advocate, my big questions would be:

  - can filename ever be unexpectedly NULL, even if type is
    CONFIG_ORIGIN_FILE?

  - do we always set filename anywhere we would have set path?

It looks like it was usually set here:

> @@ -1036,7 +1034,6 @@ static void kvi_from_source(struct config_source *cs,
>  	out->origin_type = cs->origin_type;
>  	out->linenr = cs->linenr;
>  	out->scope = scope;
> -	out->path = cs->path;
>  }

and there we'd always set out->filename from cs->name (just outside of
the context shown here). And we do so with strintern() which would
segfault if cs->name were NULL, so I think we can always depend on it. ;)

>  static int do_config_from_file(config_fn_t fn,
>  			       const enum config_origin_type origin_type,
> -			       const char *name, const char *path, FILE *f,
> -			       void *data, enum config_scope scope,
> +			       const char *name, FILE *f, void *data,
> +			       enum config_scope scope,
>  			       const struct config_options *opts)
>  {
>  	struct config_source top = CONFIG_SOURCE_INIT;
>  	int ret;
>  
> +	if (origin_type == CONFIG_ORIGIN_FILE && (!name || !*name))
> +		BUG("missing filename for CONFIG_ORIGIN_FILE");
> +

OK and here we have a sanity check that our callers will always feed an
actual name. The main caller being...

> @@ -1891,7 +1890,7 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,
>  	f = fopen_or_warn(filename, "r");
>  	if (f) {
>  		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,
> -					  filename, f, data, scope, opts);
> +					  f, data, scope, opts);
>  		fclose(f);
>  	}
>  	return ret;

...this one. Which previously passed "filename" as both the "name" and
"path" fields, so they were always identical.

Makes sense, and the patch looks good to me. Thanks for finding it!

-Peff

Re: [PATCH] config: remove unneeded struct field

[Phillip Wood]

On 16/07/2025 05:34, Jeff King wrote:
> 
> Makes sense, and the patch looks good to me. Thanks for finding it!

Thanks for the review

Phillip

Re: [PATCH] config: remove unneeded struct field

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I stumbled across this use-after-free while working on the deprecation
> of core.commentChar=auto.

And that other topic also adds more uses of "is kvi->path set?", so
we'd need a bit of semantic conflict resolution, which was fun ;-)

FYI, here is what I'd be using as merge-fix, a change that would be
squashed in when the named branch is merged in.

I expect that this part would change a lot anyway, to make it fail
when "auto" is used under WITH_BREAKING_CHANGES, so the conflict
resolution for the current/previous round may not matter all that
much.

--- >8 ---
Subject: [PATCH] merge-fix/pw/3.0-commentchar-auto-deprecation

Conflicts with pw/config-kvi-remove-path that removes kvi->path
field.  Checking if kvi->path is set should be done by inspecting
kvi->origin_type and seeing if it is CONFIG_ORIGIN_FILE instead, and
otherwise kvi->filename is usable when it is CONFIG_ORIGIN_FILE.
---
 builtin/commit.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a513709a51..04440fbd3f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -771,10 +771,10 @@ static int comment_char_config_cb(const char *key, const char *value,
 		return 0;
 
 	cfg->last_key_id = key_id;
-	if (!kvi->path) {
+	if (kvi->origin_type != CONFIG_ORIGIN_FILE) {
 		return 0;
-	} else if (get_comment_key_flags(cfg, kvi->path, key_id)) {
-		set_comment_key_flags(cfg, kvi->path, key_id, KEY_SEEN_TWICE);
+	} else if (get_comment_key_flags(cfg, kvi->filename, key_id)) {
+		set_comment_key_flags(cfg, kvi->filename, key_id, KEY_SEEN_TWICE);
 	} else {
 		struct comment_char_cfg_item *item;
 
@@ -782,8 +782,8 @@ static int comment_char_config_cb(const char *key, const char *value,
 		item = &cfg->item[cfg->nr - 1];
 		item->key_id = key_id;
 		item->scope = kvi->scope;
-		item->path = xstrdup(kvi->path);
-		set_comment_key_flags(cfg, kvi->path, key_id, KEY_SEEN_ONCE);
+		item->path = xstrdup(kvi->filename);
+		set_comment_key_flags(cfg, kvi->filename, key_id, KEY_SEEN_ONCE);
 	}
 	cfg->auto_set_in_file =	value && !strcmp(value, "auto");
 
-- 
2.50.1-441-gd7f68e2bd5

Re: [PATCH] config: remove unneeded struct field

[Phillip Wood]

On 17/07/2025 18:31, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> I stumbled across this use-after-free while working on the deprecation
>> of core.commentChar=auto.
> 
> And that other topic also adds more uses of "is kvi->path set?", so
> we'd need a bit of semantic conflict resolution, which was fun ;-)

Oh sorry I'd updated it locally and forgotten that you'd need to do the 
same.

> FYI, here is what I'd be using as merge-fix, a change that would be
> squashed in when the named branch is merged in.
> 
> I expect that this part would change a lot anyway, to make it fail
> when "auto" is used under WITH_BREAKING_CHANGES, so the conflict
> resolution for the current/previous round may not matter all that
> much.

This part of the code gets moved but doesn't really change. Your 
resolution looks good. I'm mostly there with the re-roll but it will be 
next week before it is ready to post to the.

Thanks

Phillip

> 
> --- >8 ---
> Subject: [PATCH] merge-fix/pw/3.0-commentchar-auto-deprecation
> 
> Conflicts with pw/config-kvi-remove-path that removes kvi->path
> field.  Checking if kvi->path is set should be done by inspecting
> kvi->origin_type and seeing if it is CONFIG_ORIGIN_FILE instead, and
> otherwise kvi->filename is usable when it is CONFIG_ORIGIN_FILE.
> ---
>   builtin/commit.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index a513709a51..04440fbd3f 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -771,10 +771,10 @@ static int comment_char_config_cb(const char *key, const char *value,
>   		return 0;
>   
>   	cfg->last_key_id = key_id;
> -	if (!kvi->path) {
> +	if (kvi->origin_type != CONFIG_ORIGIN_FILE) {
>   		return 0;
> -	} else if (get_comment_key_flags(cfg, kvi->path, key_id)) {
> -		set_comment_key_flags(cfg, kvi->path, key_id, KEY_SEEN_TWICE);
> +	} else if (get_comment_key_flags(cfg, kvi->filename, key_id)) {
> +		set_comment_key_flags(cfg, kvi->filename, key_id, KEY_SEEN_TWICE);
>   	} else {
>   		struct comment_char_cfg_item *item;
>   
> @@ -782,8 +782,8 @@ static int comment_char_config_cb(const char *key, const char *value,
>   		item = &cfg->item[cfg->nr - 1];
>   		item->key_id = key_id;
>   		item->scope = kvi->scope;
> -		item->path = xstrdup(kvi->path);
> -		set_comment_key_flags(cfg, kvi->path, key_id, KEY_SEEN_ONCE);
> +		item->path = xstrdup(kvi->filename);
> +		set_comment_key_flags(cfg, kvi->filename, key_id, KEY_SEEN_ONCE);
>   	}
>   	cfg->auto_set_in_file =	value && !strcmp(value, "auto");
>