config: remove unneeded struct field

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/config.c b/config.c
index b18b5617fcd05d146aec7b44edd50ff53523be39..85883afb6c58bb941ca0b96817ab549293e66aff 100644 (file)
--- 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,
@@ -1855,17 +1852,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;
@@ -1880,8 +1879,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,
@@ -1896,7 +1895,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;
@@ -1921,7 +1920,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 29a027748375f1fedf31384ada7a3f70a435aa4c..cbb0f4fddcddec73ef61c65f8e9e88e496bc69dc 100644 (file)
--- 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. */