{"thread":{"id":"63796","subject":"[PATCH] config: remove unneeded struct field","startedAt":"2025-07-15T14:01:22Z","lastAt":"2025-07-18T14:40:33Z","messageCount":5,"participants":["Phillip Wood","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"521997","messageId":"31724ce432b19088b7c7643654788d8e712b3193.1752588042.git.phillip.wood@dunelm.org.uk","threadId":"63796","inReplyTo":null,"subject":"[PATCH] config: remove unneeded struct field","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-07-15T14:00:56Z","receivedAt":"2025-07-15T14:01:22Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nAs well as receiving the config key and value, config callbacks\nalso receive a \"struct key_value_info\" containing information about\nthe source of the key-value pair. Accessing the \"path\" field of\nthis struct from a callback passed to repo_config() results in a\nuse-after-free. This happens because repo_config() first populates a\nconfigset by calling config_with_options() and then iterates over the\nconfigset with the callback passed by the caller. When the configset\nis constructed it takes a shallow copy of the \"struct key_value_info\"\nfor each config setting. This leads to the use-after-free as the\n\"path\" member is freed before config_with_options() returns.\n\nWe could fix this by interning the \"path\" field as we do\nfor the \"filename\" field but the \"path\" field is not actually\nneeded. It is populated with a copy of the \"path\" field from \"struct\nconfig_source\". That field was added in d14d42440d8 (config: disallow\nrelative include paths from blobs, 2014-02-19) to distinguish between\nrelative include directives in files and those in blobs. However,\nsince 1b8132d99d8 (i18n: config: unfold error messages marked for\ntranslation, 2016-07-28) we can differentiate these by looking at the\n\"origin_type\" field in \"struct key_value_info\". So let's remove the\n\"path\" members from \"struct config_source\" and \"struct key_value_info\"\nand instead use a combination of the \"filename\" and \"origin_type\"\nfields to determine the absolute path of relative includes.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\nI stumbled across this use-after-free while working on the deprecation\nof core.commentChar=auto.\nBase-Commit: a30f80fde927d70950b3b4d1820813480968fb0d\nPublished-As: https://github.com/phillipwood/git/releases/tag/pw%2Fconfig-remove-kvi-path%2Fv1\nView-Changes-At: https://github.com/phillipwood/git/compare/a30f80fde...31724ce43\nFetch-It-Via: git fetch https://github.com/phillipwood/git pw/config-remove-kvi-path/v1\n\n config.c | 28 +++++++++++++---------------\n config.h |  2 --\n 2 files changed, 13 insertions(+), 17 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex f55508bdc21..479ac7bf1e8 100644\n--- a/config.c\n+++ b/config.c\n@@ -56,7 +56,6 @@ struct config_source {\n \t} u;\n \tenum config_origin_type origin_type;\n \tconst char *name;\n-\tconst char *path;\n \tenum config_error_action default_error_action;\n \tint linenr;\n \tint eof;\n@@ -173,14 +172,14 @@ static int handle_path_include(const struct key_value_info *kvi,\n \tif (!is_absolute_path(path)) {\n \t\tchar *slash;\n \n-\t\tif (!kvi || !kvi->path) {\n+\t\tif (!kvi || kvi->origin_type != CONFIG_ORIGIN_FILE) {\n \t\t\tret = error(_(\"relative config includes must come from files\"));\n \t\t\tgoto cleanup;\n \t\t}\n \n-\t\tslash = find_last_dir_sep(kvi->path);\n+\t\tslash = find_last_dir_sep(kvi->filename);\n \t\tif (slash)\n-\t\t\tstrbuf_add(&buf, kvi->path, slash - kvi->path + 1);\n+\t\t\tstrbuf_add(&buf, kvi->filename, slash - kvi->filename + 1);\n \t\tstrbuf_addstr(&buf, path);\n \t\tpath = buf.buf;\n \t}\n@@ -224,11 +223,11 @@ static int prepare_include_condition_pattern(const struct key_value_info *kvi,\n \tif (pat->buf[0] == '.' && is_dir_sep(pat->buf[1])) {\n \t\tconst char *slash;\n \n-\t\tif (!kvi || !kvi->path)\n+\t\tif (!kvi || kvi->origin_type != CONFIG_ORIGIN_FILE)\n \t\t\treturn error(_(\"relative config include \"\n \t\t\t\t       \"conditionals must come from files\"));\n \n-\t\tstrbuf_realpath(&path, kvi->path, 1);\n+\t\tstrbuf_realpath(&path, kvi->filename, 1);\n \t\tslash = find_last_dir_sep(path.buf);\n \t\tif (!slash)\n \t\t\tBUG(\"how is this possible?\");\n@@ -633,7 +632,6 @@ void kvi_from_param(struct key_value_info *out)\n \tout->linenr = -1;\n \tout->origin_type = CONFIG_ORIGIN_CMDLINE;\n \tout->scope = CONFIG_SCOPE_COMMAND;\n-\tout->path = NULL;\n }\n \n int git_config_parse_parameter(const char *text,\n@@ -1036,7 +1034,6 @@ static void kvi_from_source(struct config_source *cs,\n \tout->origin_type = cs->origin_type;\n \tout->linenr = cs->linenr;\n \tout->scope = scope;\n-\tout->path = cs->path;\n }\n \n static int git_parse_source(struct config_source *cs, config_fn_t fn,\n@@ -1850,17 +1847,19 @@ static int do_config_from(struct config_source *top, config_fn_t fn,\n \n static int do_config_from_file(config_fn_t fn,\n \t\t\t       const enum config_origin_type origin_type,\n-\t\t\t       const char *name, const char *path, FILE *f,\n-\t\t\t       void *data, enum config_scope scope,\n+\t\t\t       const char *name, FILE *f, void *data,\n+\t\t\t       enum config_scope scope,\n \t\t\t       const struct config_options *opts)\n {\n \tstruct config_source top = CONFIG_SOURCE_INIT;\n \tint ret;\n \n+\tif (origin_type == CONFIG_ORIGIN_FILE && (!name || !*name))\n+\t\tBUG(\"missing filename for CONFIG_ORIGIN_FILE\");\n+\n \ttop.u.file = f;\n \ttop.origin_type = origin_type;\n \ttop.name = name;\n-\ttop.path = path;\n \ttop.default_error_action = CONFIG_ERROR_DIE;\n \ttop.do_fgetc = config_file_fgetc;\n \ttop.do_ungetc = config_file_ungetc;\n@@ -1875,8 +1874,8 @@ static int do_config_from_file(config_fn_t fn,\n static int git_config_from_stdin(config_fn_t fn, void *data,\n \t\t\t\t enum config_scope scope)\n {\n-\treturn do_config_from_file(fn, CONFIG_ORIGIN_STDIN, \"\", NULL, stdin,\n-\t\t\t\t   data, scope, NULL);\n+\treturn do_config_from_file(fn, CONFIG_ORIGIN_STDIN, \"\", stdin, data,\n+\t\t\t\t   scope, NULL);\n }\n \n int git_config_from_file_with_options(config_fn_t fn, const char *filename,\n@@ -1891,7 +1890,7 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,\n \tf = fopen_or_warn(filename, \"r\");\n \tif (f) {\n \t\tret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,\n-\t\t\t\t\t  filename, f, data, scope, opts);\n+\t\t\t\t\t  f, data, scope, opts);\n \t\tfclose(f);\n \t}\n \treturn ret;\n@@ -1916,7 +1915,6 @@ int git_config_from_mem(config_fn_t fn,\n \ttop.u.buf.pos = 0;\n \ttop.origin_type = origin_type;\n \ttop.name = name;\n-\ttop.path = NULL;\n \ttop.default_error_action = CONFIG_ERROR_ERROR;\n \ttop.do_fgetc = config_buf_fgetc;\n \ttop.do_ungetc = config_buf_ungetc;\ndiff --git a/config.h b/config.h\nindex 29a02774837..cbb0f4fddcd 100644\n--- a/config.h\n+++ b/config.h\n@@ -122,14 +122,12 @@ struct key_value_info {\n \tint linenr;\n \tenum config_origin_type origin_type;\n \tenum config_scope scope;\n-\tconst char *path;\n };\n #define KVI_INIT { \\\n \t.filename = NULL, \\\n \t.linenr = -1, \\\n \t.origin_type = CONFIG_ORIGIN_UNKNOWN, \\\n \t.scope = CONFIG_SCOPE_UNKNOWN, \\\n-\t.path = NULL, \\\n }\n \n /* Captures additional information that a config callback can use. */\n-- \n2.49.0.897.gfad3eb7d210\n\n"},{"id":"522062","messageId":"20250716043455.GA1394788@coredump.intra.peff.net","threadId":"63796","inReplyTo":"31724ce432b19088b7c7643654788d8e712b3193.1752588042.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH] config: remove unneeded struct field","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-16T04:34:55Z","receivedAt":"2025-07-16T04:35:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 15, 2025 at 03:00:56PM +0100, Phillip Wood wrote:\n\n> We could fix this by interning the \"path\" field as we do\n> for the \"filename\" field but the \"path\" field is not actually\n> needed. It is populated with a copy of the \"path\" field from \"struct\n> config_source\". That field was added in d14d42440d8 (config: disallow\n> relative include paths from blobs, 2014-02-19) to distinguish between\n> relative include directives in files and those in blobs. However,\n> since 1b8132d99d8 (i18n: config: unfold error messages marked for\n> translation, 2016-07-28) we can differentiate these by looking at the\n> \"origin_type\" field in \"struct key_value_info\". So let's remove the\n> \"path\" members from \"struct config_source\" and \"struct key_value_info\"\n> and instead use a combination of the \"filename\" and \"origin_type\"\n> fields to determine the absolute path of relative includes.\n\nNicely explained. The interesting bit of the patch is here:\n\n> @@ -173,14 +172,14 @@ static int handle_path_include(const struct key_value_info *kvi,\n>  \tif (!is_absolute_path(path)) {\n>  \t\tchar *slash;\n>  \n> -\t\tif (!kvi || !kvi->path) {\n> +\t\tif (!kvi || kvi->origin_type != CONFIG_ORIGIN_FILE) {\n>  \t\t\tret = error(_(\"relative config includes must come from files\"));\n>  \t\t\tgoto cleanup;\n>  \t\t}\n\n...which is where we no longer need the separate variable to make the\npath/filename distinction.\n\nJust playing devil's advocate, my big questions would be:\n\n  - can filename ever be unexpectedly NULL, even if type is\n    CONFIG_ORIGIN_FILE?\n\n  - do we always set filename anywhere we would have set path?\n\nIt looks like it was usually set here:\n\n> @@ -1036,7 +1034,6 @@ static void kvi_from_source(struct config_source *cs,\n>  \tout->origin_type = cs->origin_type;\n>  \tout->linenr = cs->linenr;\n>  \tout->scope = scope;\n> -\tout->path = cs->path;\n>  }\n\nand there we'd always set out->filename from cs->name (just outside of\nthe context shown here). And we do so with strintern() which would\nsegfault if cs->name were NULL, so I think we can always depend on it. ;)\n\n>  static int do_config_from_file(config_fn_t fn,\n>  \t\t\t       const enum config_origin_type origin_type,\n> -\t\t\t       const char *name, const char *path, FILE *f,\n> -\t\t\t       void *data, enum config_scope scope,\n> +\t\t\t       const char *name, FILE *f, void *data,\n> +\t\t\t       enum config_scope scope,\n>  \t\t\t       const struct config_options *opts)\n>  {\n>  \tstruct config_source top = CONFIG_SOURCE_INIT;\n>  \tint ret;\n>  \n> +\tif (origin_type == CONFIG_ORIGIN_FILE && (!name || !*name))\n> +\t\tBUG(\"missing filename for CONFIG_ORIGIN_FILE\");\n> +\n\nOK and here we have a sanity check that our callers will always feed an\nactual name. The main caller being...\n\n> @@ -1891,7 +1890,7 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,\n>  \tf = fopen_or_warn(filename, \"r\");\n>  \tif (f) {\n>  \t\tret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,\n> -\t\t\t\t\t  filename, f, data, scope, opts);\n> +\t\t\t\t\t  f, data, scope, opts);\n>  \t\tfclose(f);\n>  \t}\n>  \treturn ret;\n\n...this one. Which previously passed \"filename\" as both the \"name\" and\n\"path\" fields, so they were always identical.\n\nMakes sense, and the patch looks good to me. Thanks for finding it!\n\n-Peff\n"},{"id":"522088","messageId":"a7aceebe-e2fa-42f2-b36d-0ef692d148e9@gmail.com","threadId":"63796","inReplyTo":"20250716043455.GA1394788@coredump.intra.peff.net","subject":"Re: [PATCH] config: remove unneeded struct field","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-07-16T09:41:15Z","receivedAt":"2025-07-16T09:41:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 16/07/2025 05:34, Jeff King wrote:\n> \n> Makes sense, and the patch looks good to me. Thanks for finding it!\n\nThanks for the review\n\nPhillip\n\n"},{"id":"522223","messageId":"xmqqv7nq67wr.fsf@gitster.g","threadId":"63796","inReplyTo":"31724ce432b19088b7c7643654788d8e712b3193.1752588042.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH] config: remove unneeded struct field","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-17T17:31:32Z","receivedAt":"2025-07-17T17:31:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> I stumbled across this use-after-free while working on the deprecation\n> of core.commentChar=auto.\n\nAnd that other topic also adds more uses of \"is kvi->path set?\", so\nwe'd need a bit of semantic conflict resolution, which was fun ;-)\n\nFYI, here is what I'd be using as merge-fix, a change that would be\nsquashed in when the named branch is merged in.\n\nI expect that this part would change a lot anyway, to make it fail\nwhen \"auto\" is used under WITH_BREAKING_CHANGES, so the conflict\nresolution for the current/previous round may not matter all that\nmuch.\n\n--- >8 ---\nSubject: [PATCH] merge-fix/pw/3.0-commentchar-auto-deprecation\n\nConflicts with pw/config-kvi-remove-path that removes kvi->path\nfield.  Checking if kvi->path is set should be done by inspecting\nkvi->origin_type and seeing if it is CONFIG_ORIGIN_FILE instead, and\notherwise kvi->filename is usable when it is CONFIG_ORIGIN_FILE.\n---\n builtin/commit.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex a513709a51..04440fbd3f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -771,10 +771,10 @@ static int comment_char_config_cb(const char *key, const char *value,\n \t\treturn 0;\n \n \tcfg->last_key_id = key_id;\n-\tif (!kvi->path) {\n+\tif (kvi->origin_type != CONFIG_ORIGIN_FILE) {\n \t\treturn 0;\n-\t} else if (get_comment_key_flags(cfg, kvi->path, key_id)) {\n-\t\tset_comment_key_flags(cfg, kvi->path, key_id, KEY_SEEN_TWICE);\n+\t} else if (get_comment_key_flags(cfg, kvi->filename, key_id)) {\n+\t\tset_comment_key_flags(cfg, kvi->filename, key_id, KEY_SEEN_TWICE);\n \t} else {\n \t\tstruct comment_char_cfg_item *item;\n \n@@ -782,8 +782,8 @@ static int comment_char_config_cb(const char *key, const char *value,\n \t\titem = &cfg->item[cfg->nr - 1];\n \t\titem->key_id = key_id;\n \t\titem->scope = kvi->scope;\n-\t\titem->path = xstrdup(kvi->path);\n-\t\tset_comment_key_flags(cfg, kvi->path, key_id, KEY_SEEN_ONCE);\n+\t\titem->path = xstrdup(kvi->filename);\n+\t\tset_comment_key_flags(cfg, kvi->filename, key_id, KEY_SEEN_ONCE);\n \t}\n \tcfg->auto_set_in_file =\tvalue && !strcmp(value, \"auto\");\n \n-- \n2.50.1-441-gd7f68e2bd5\n\n"},{"id":"522282","messageId":"2ff3a900-daea-4264-8509-70464952f74f@gmail.com","threadId":"63796","inReplyTo":"xmqqv7nq67wr.fsf@gitster.g","subject":"Re: [PATCH] config: remove unneeded struct field","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-07-18T14:40:30Z","receivedAt":"2025-07-18T14:40:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 17/07/2025 18:31, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>> I stumbled across this use-after-free while working on the deprecation\n>> of core.commentChar=auto.\n> \n> And that other topic also adds more uses of \"is kvi->path set?\", so\n> we'd need a bit of semantic conflict resolution, which was fun ;-)\n\nOh sorry I'd updated it locally and forgotten that you'd need to do the \nsame.\n\n> FYI, here is what I'd be using as merge-fix, a change that would be\n> squashed in when the named branch is merged in.\n> \n> I expect that this part would change a lot anyway, to make it fail\n> when \"auto\" is used under WITH_BREAKING_CHANGES, so the conflict\n> resolution for the current/previous round may not matter all that\n> much.\n\nThis part of the code gets moved but doesn't really change. Your \nresolution looks good. I'm mostly there with the re-roll but it will be \nnext week before it is ready to post to the.\n\nThanks\n\nPhillip\n\n> \n> --- >8 ---\n> Subject: [PATCH] merge-fix/pw/3.0-commentchar-auto-deprecation\n> \n> Conflicts with pw/config-kvi-remove-path that removes kvi->path\n> field.  Checking if kvi->path is set should be done by inspecting\n> kvi->origin_type and seeing if it is CONFIG_ORIGIN_FILE instead, and\n> otherwise kvi->filename is usable when it is CONFIG_ORIGIN_FILE.\n> ---\n>   builtin/commit.c | 10 +++++-----\n>   1 file changed, 5 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index a513709a51..04440fbd3f 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -771,10 +771,10 @@ static int comment_char_config_cb(const char *key, const char *value,\n>   \t\treturn 0;\n>   \n>   \tcfg->last_key_id = key_id;\n> -\tif (!kvi->path) {\n> +\tif (kvi->origin_type != CONFIG_ORIGIN_FILE) {\n>   \t\treturn 0;\n> -\t} else if (get_comment_key_flags(cfg, kvi->path, key_id)) {\n> -\t\tset_comment_key_flags(cfg, kvi->path, key_id, KEY_SEEN_TWICE);\n> +\t} else if (get_comment_key_flags(cfg, kvi->filename, key_id)) {\n> +\t\tset_comment_key_flags(cfg, kvi->filename, key_id, KEY_SEEN_TWICE);\n>   \t} else {\n>   \t\tstruct comment_char_cfg_item *item;\n>   \n> @@ -782,8 +782,8 @@ static int comment_char_config_cb(const char *key, const char *value,\n>   \t\titem = &cfg->item[cfg->nr - 1];\n>   \t\titem->key_id = key_id;\n>   \t\titem->scope = kvi->scope;\n> -\t\titem->path = xstrdup(kvi->path);\n> -\t\tset_comment_key_flags(cfg, kvi->path, key_id, KEY_SEEN_ONCE);\n> +\t\titem->path = xstrdup(kvi->filename);\n> +\t\tset_comment_key_flags(cfg, kvi->filename, key_id, KEY_SEEN_ONCE);\n>   \t}\n>   \tcfg->auto_set_in_file =\tvalue && !strcmp(value, \"auto\");\n>   \n\n\n"}]}