threads / patch / 63796

patchconfig: remove unneeded struct field

Subject: [PATCH] config: remove unneeded struct field

## tl;dr

5 messages between Jul 15, 2025 and Jul 18, 2025. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Phillip Wood· Jul 15, 2025, 14:00 UTC · lore
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(-)
Show changes to 2 files +13 −17

config.c, config.h

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
Jeff King· Jul 16, 2025, 04:34 UTC · re: Phillip Wood · lore

Re: [PATCH] config: remove unneeded struct field

On Tue, Jul 15, 2025 at 03:00:56PM +0100, Phillip Wood wrote:
Show 12 quoted lines
> 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:
Show 9 quoted lines
> @@ -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:
Show 6 quoted lines
> @@ -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. ;)

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

Show 9 quoted lines
> @@ -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
Junio C Hamano· Jul 17, 2025, 17:31 UTC · re: Phillip Wood · lore

Re: [PATCH] config: remove unneeded struct field

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(-)
Show changes to builtin/commit.c +5 −5
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
Phillip Wood· Jul 18, 2025, 14:40 UTC · re: Junio C Hamano · lore

Re: [PATCH] config: remove unneeded struct field

On 17/07/2025 18:31, Junio C Hamano wrote:
Show 7 quoted lines
> 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.

Show 7 quoted lines
> 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
Show 41 quoted lines
> 
> --- >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");
>

← back to recent threads