From b09c53a3e331211fc0154de8ebb271e48f8c7ee5 Mon Sep 17 00:00:00 2001 From: Libor Pechacek Date: Sun, 30 Jan 2011 20:40:41 +0100 Subject: Sanity-check config variable names Sanity-check config variable names when adding and retrieving them. As a side effect code duplication between git_config_set_multivar and get_value (in builtin/config.c) was removed and the common functionality was placed in git_config_parse_key. This breaks a test in t1300 which used invalid section-less keys in the tests for "git -c". However, allowing such names there was useless, since there was no way to set them via config file, and no part of git actually tried to use section-less keys. This patch updates the test to use more realistic examples as well as adding its own test. Signed-off-by: Libor Pechacek Acked-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/config.c | 27 +++++++++---- cache.h | 1 + config.c | 106 +++++++++++++++++++++++++++++++------------------ t/t1300-repo-config.sh | 18 +++++++-- 4 files changed, 102 insertions(+), 50 deletions(-) diff --git a/builtin/config.c b/builtin/config.c index ca4a0db4a..6754da8f5 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -153,7 +153,6 @@ static int show_config(const char *key_, const char *value_, void *cb) static int get_value(const char *key_, const char *regex_) { int ret = -1; - char *tl; char *global = NULL, *repo_config = NULL; const char *system_wide = NULL, *local; @@ -167,18 +166,32 @@ static int get_value(const char *key_, const char *regex_) system_wide = git_etc_gitconfig(); } - key = xstrdup(key_); - for (tl=key+strlen(key)-1; tl >= key && *tl != '.'; --tl) - *tl = tolower(*tl); - for (tl=key; *tl && *tl != '.'; ++tl) - *tl = tolower(*tl); - if (use_key_regexp) { + char *tl; + + /* + * NEEDSWORK: this naive pattern lowercasing obviously does not + * work for more complex patterns like "^[^.]*Foo.*bar". + * Perhaps we should deprecate this altogether someday. + */ + + key = xstrdup(key_); + for (tl = key + strlen(key) - 1; + tl >= key && *tl != '.'; + tl--) + *tl = tolower(*tl); + for (tl = key; *tl && *tl != '.'; tl++) + *tl = tolower(*tl); + key_regexp = (regex_t*)xmalloc(sizeof(regex_t)); if (regcomp(key_regexp, key, REG_EXTENDED)) { fprintf(stderr, "Invalid key pattern: %s\n", key_); + free(key); goto free_strings; } + } else { + if (git_config_parse_key(key_, &key, NULL)) + goto free_strings; } if (regex_) { diff --git a/cache.h b/cache.h index d83d68c85..1e32d63fb 100644 --- a/cache.h +++ b/cache.h @@ -997,6 +997,7 @@ extern int git_config_maybe_bool(const char *, const char *); extern int git_config_string(const char **, const char *, const char *); extern int git_config_pathname(const char **, const char *, const char *); extern int git_config_set(const char *, const char *); +extern int git_config_parse_key(const char *, char **, int *); extern int git_config_set_multivar(const char *, const char *, const char *, int); extern int git_config_rename_section(const char *, const char *); extern const char *git_etc_gitconfig(void); diff --git a/config.c b/config.c index 625e05187..f758734d4 100644 --- a/config.c +++ b/config.c @@ -1098,6 +1098,70 @@ int git_config_set(const char *key, const char *value) return git_config_set_multivar(key, value, NULL, 0); } +/* + * Auxiliary function to sanity-check and split the key into the section + * identifier and variable name. + * + * Returns 0 on success, -1 when there is an invalid character in the key and + * -2 if there is no section name in the key. + * + * store_key - pointer to char* which will hold a copy of the key with + * lowercase section and variable name + * baselen - pointer to int which will hold the length of the + * section + subsection part, can be NULL + */ +int git_config_parse_key(const char *key, char **store_key, int *baselen_) +{ + int i, dot, baselen; + const char *last_dot = strrchr(key, '.'); + + /* + * Since "key" actually contains the section name and the real + * key name separated by a dot, we have to know where the dot is. + */ + + if (last_dot == NULL) { + error("key does not contain a section: %s", key); + return -2; + } + + baselen = last_dot - key; + if (baselen_) + *baselen_ = baselen; + + /* + * Validate the key and while at it, lower case it for matching. + */ + *store_key = xmalloc(strlen(key) + 1); + + dot = 0; + for (i = 0; key[i]; i++) { + unsigned char c = key[i]; + if (c == '.') + dot = 1; + /* Leave the extended basename untouched.. */ + if (!dot || i > baselen) { + if (!iskeychar(c) || + (i == baselen + 1 && !isalpha(c))) { + error("invalid key: %s", key); + goto out_free_ret_1; + } + c = tolower(c); + } else if (c == '\n') { + error("invalid key (newline): %s", key); + goto out_free_ret_1; + } + (*store_key)[i] = c; + } + (*store_key)[i] = 0; + + return 0; + +out_free_ret_1: + free(*store_key); + return -1; +} + /* * If value==NULL, unset in (remove from) config, * if value_regex!=NULL, disregard key/value pairs where value does not match. @@ -1124,59 +1188,23 @@ int git_config_set(const char *key, const char *value) int git_config_set_multivar(const char *key, const char *value, const char *value_regex, int multi_replace) { - int i, dot; int fd = -1, in_fd; int ret; char *config_filename; struct lock_file *lock = NULL; - const char *last_dot = strrchr(key, '.'); if (config_exclusive_filename) config_filename = xstrdup(config_exclusive_filename); else config_filename = git_pathdup("config"); - /* - * Since "key" actually contains the section name and the real - * key name separated by a dot, we have to know where the dot is. - */ - - if (last_dot == NULL) { - error("key does not contain a section: %s", key); - ret = 2; + /* parse-key returns negative; flip the sign to feed exit(3) */ + ret = 0 - git_config_parse_key(key, &store.key, &store.baselen); + if (ret) goto out_free; - } - store.baselen = last_dot - key; store.multi_replace = multi_replace; - /* - * Validate the key and while at it, lower case it for matching. - */ - store.key = xmalloc(strlen(key) + 1); - dot = 0; - for (i = 0; key[i]; i++) { - unsigned char c = key[i]; - if (c == '.') - dot = 1; - /* Leave the extended basename untouched.. */ - if (!dot || i > store.baselen) { - if (!iskeychar(c) || (i == store.baselen+1 && !isalpha(c))) { - error("invalid key: %s", key); - free(store.key); - ret = 1; - goto out_free; - } - c = tolower(c); - } else if (c == '\n') { - error("invalid key (newline): %s", key); - free(store.key); - ret = 1; - goto out_free; - } - store.key[i] = c; - } - store.key[i] = 0; /* * The lock serves a purpose in addition to locking: the new diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index d0e55465f..c3d91d10a 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -876,11 +876,21 @@ test_expect_success 'check split_cmdline return' " " test_expect_success 'git -c "key=value" support' ' - test "z$(git -c name=value config name)" = zvalue && test "z$(git -c core.name=value config core.name)" = zvalue && - test "z$(git -c CamelCase=value config camelcase)" = zvalue && - test "z$(git -c flag config --bool flag)" = ztrue && - test_must_fail git -c core.name=value config name + test "z$(git -c foo.CamelCase=value config foo.camelcase)" = zvalue && + test "z$(git -c foo.flag config --bool foo.flag)" = ztrue && + test_must_fail git -c name=value config core.name +' + +test_expect_success 'key sanity-checking' ' + test_must_fail git config foo=bar && + test_must_fail git config foo=.bar && + test_must_fail git config foo.ba=r && + test_must_fail git config foo.1bar && + test_must_fail git config foo."ba + z".bar && + git config foo.bar true && + git config foo."ba =z".bar false ' test_done -- cgit v1.2.1 From 2169ddc056b56deba701cbbba28cdaf2e9821224 Mon Sep 17 00:00:00 2001 From: Libor Pechacek Date: Tue, 1 Feb 2011 08:13:47 +0100 Subject: Disallow empty section and variable names It is possible to break your repository config by creating an invalid key. The config parser in turn chokes on it: $ git init Initialized empty Git repository in /tmp/gittest/.git/ $ git config .foo false $ git config core.bare fatal: bad config file line 6 in .git/config This patch makes git-config reject keys which start or end with a dot and adds tests for these cases. Signed-off-by: Libor Pechacek Signed-off-by: Junio C Hamano --- config.c | 7 ++++++- t/t1300-repo-config.sh | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index f758734d4..d5bb8629a 100644 --- a/config.c +++ b/config.c @@ -1120,11 +1120,16 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_) * key name separated by a dot, we have to know where the dot is. */ - if (last_dot == NULL) { + if (last_dot == NULL || last_dot == key) { error("key does not contain a section: %s", key); return -2; } + if (!last_dot[1]) { + error("key does not contain variable name: %s", key); + return -2; + } + baselen = last_dot - key; if (baselen_) *baselen_ = baselen; diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index c3d91d10a..53fb8228c 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -889,6 +889,10 @@ test_expect_success 'key sanity-checking' ' test_must_fail git config foo.1bar && test_must_fail git config foo."ba z".bar && + test_must_fail git config . false && + test_must_fail git config .foo false && + test_must_fail git config foo. false && + test_must_fail git config .foo. false && git config foo.bar true && git config foo."ba =z".bar false ' -- cgit v1.2.1