From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> --- 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
