threads / patch / 26349

patchDisallow empty section and variable names

Subject: [PATCH] Disallow empty section and variable names

## tl;dr

9 messages between Jan 27, 2011 and Feb 10, 2011. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Libor Pechacek· Jan 27, 2011, 14:52 UTC · lore

It is possible to break you repository config by creating an invalid config option. The config parser in turn chokes on it.

$ git init Initialized empty Git repository in /tmp/gittest/.git/ $ git config .foo false $ git config .foo fatal: bad config file line 6 in .git/config

This patch makes git-config reject keys which start or end with a dot. The fix also revealed a typo in t5526-fetch-submodules, which is fixed by this patch as well.

Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
---
Applies on top "Sanity-check config variable names".
 config.c                    |    7 ++++++-
 t/t5526-fetch-submodules.sh |    2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)
Show changes to 2 files +7 −2

config.c, t/t5526-fetch-submodules.sh

diff --git a/config.c b/config.c
index c976544..81a0705 100644
--- a/config.c
+++ b/config.c
@@ -1119,7 +1119,7 @@ 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 || *key == '.') {
 		error("key does not contain a section: %s", key);
 		return -2;
 	}
@@ -1156,6 +1156,11 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	if (store_key)
 		(*store_key)[i] = 0;
 
+	if (key[i-1] == '.') {
+		error("key does not contain variable name: %s", key);
+		goto out_free_ret_1;
+	}
+
 	return 0;
 
 out_free_ret_1:
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 884a5e5..7106c6c 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -124,7 +124,7 @@ test_expect_success "--recurse-submodules overrides fetchRecurseSubmodules setti
 	(
 		cd downstream &&
 		git fetch --recurse-submodules >../actual.out 2>../actual.err &&
-		git config -f --unset .gitmodules submodule.submodule.fetchRecurseSubmodules true &&
+		git config -f .gitmodules --unset submodule.submodule.fetchRecurseSubmodules true &&
 		git config --unset submodule.submodule.fetchRecurseSubmodules
 	) &&
 	test_cmp expect.out actual.out &&

-- 
1.7.4.rc3.3.g8b2bfe
Libor Pechacek· Jan 30, 2011, 20:34 UTC · re: Libor Pechacek · lore

[PATCH v2] 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 .foo fatal: bad config file line 6 in .git/config

This patch makes git-config reject keys which start or end with a dot, adds tests for these cases and also fixes a typo in t5526-fetch-submodules, which was exposed by the new check.

Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
---

Added tests for the cases checked, made git_config_parse_key consistently return -2 when the key is invalid. Applies on top "Sanity-check config variable names".

 config.c                    |    8 +++++++-
 t/t1300-repo-config.sh      |    4 ++++
 t/t5526-fetch-submodules.sh |    2 +-
 3 files changed, 12 insertions(+), 2 deletions(-)
Show changes to 3 files +12 −2

config.c, t/t1300-repo-config.sh, t/t5526-fetch-submodules.sh

diff --git a/config.c b/config.c
index fde91f5..e27a39f 100644
--- a/config.c
+++ b/config.c
@@ -1120,11 +1120,17 @@ 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 || *key == '.') {
 		error("key does not contain a section: %s", key);
 		return -2;
 	}
 
+	i = strlen(key);
+	if (i && key[i-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 c3d91d1..568d51d 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 . &&
+	test_must_fail git config .foo &&
+	test_must_fail git config foo. &&
+	test_must_fail git config .foo. &&
 	git config foo.bar true &&
 	git config foo."ba =z".bar false
 '
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 884a5e5..7106c6c 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -124,7 +124,7 @@ test_expect_success "--recurse-submodules overrides fetchRecurseSubmodules setti
 	(
 		cd downstream &&
 		git fetch --recurse-submodules >../actual.out 2>../actual.err &&
-		git config -f --unset .gitmodules submodule.submodule.fetchRecurseSubmodules true &&
+		git config -f .gitmodules --unset submodule.submodule.fetchRecurseSubmodules true &&
 		git config --unset submodule.submodule.fetchRecurseSubmodules
 	) &&
 	test_cmp expect.out actual.out &&

-- 
1.7.4.rc3.11.g54760
Johannes Sixt· Jan 31, 2011, 07:48 UTC · re: Libor Pechacek · lore

Re: [PATCH v2] Disallow empty section and variable names

Am 1/30/2011 21:34, schrieb Libor Pechacek:
Show 8 quoted lines
> 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 .foo
> fatal: bad config file line 6 in .git/config

Just a nit: Your example is misleading because it "only" shows that if you shove in junk, and ask for junk, you get breakage. However, the breakage is much more serious, and you could have demonstrated it, if you had written your example like this:

$ git config .foo false $ git config user.email fatal: bad config file line 6 in .git/config

> +	test_must_fail git config . &&
> +	test_must_fail git config .foo &&
> +	test_must_fail git config foo. &&
> +	test_must_fail git config .foo. &&

Not a nit: These tests only show that 'git config' cannot be asked for junk, but they do not show that you cannot insert junk into the config file anymore using 'git config'.

-- Hannes
Libor Pechacek· Jan 31, 2011, 09:17 UTC · re: Johannes Sixt · lore

Re: [PATCH v2] Disallow empty section and variable names

On Mon 31-01-11 08:48:31, Johannes Sixt wrote:
Show 18 quoted lines
> Am 1/30/2011 21:34, schrieb Libor Pechacek:
> > 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 .foo
> > fatal: bad config file line 6 in .git/config
> 
> Just a nit: Your example is misleading because it "only" shows that if you
> shove in junk, and ask for junk, you get breakage. However, the breakage
> is much more serious, and you could have demonstrated it, if you had
> written your example like this:
> 
> $ git config .foo false
> $ git config user.email
> fatal: bad config file line 6 in .git/config
Yes, that one is more persuasive.  Thanks for hint.
Show 8 quoted lines
> > +	test_must_fail git config . &&
> > +	test_must_fail git config .foo &&
> > +	test_must_fail git config foo. &&
> > +	test_must_fail git config .foo. &&
> 
> Not a nit: These tests only show that 'git config' cannot be asked for
> junk, but they do not show that you cannot insert junk into the config
> file anymore using 'git config'.

This change builds on top of "Sanity-check config variable names" which makes setting and getting values use the same key checking routine. For the moment, it does not matter if we test the "set" ot "get" path.

Yet, I accept the get/set operations can use different key checking routine in the future (although I can't imagine the reson for it), and overall testing the "set" path is in line with the idea of the patch.

Thanks for input, I'll send updated patch.
Libor
-- 
Libor Pechacek
SUSE L3 Team, Prague
Johannes Sixt· Jan 31, 2011, 09:29 UTC · re: Libor Pechacek · lore

Re: [PATCH v2] Disallow empty section and variable names

Am 1/31/2011 10:17, schrieb Libor Pechacek:
Show 14 quoted lines
> On Mon 31-01-11 08:48:31, Johannes Sixt wrote:
>> Am 1/30/2011 21:34, schrieb Libor Pechacek:
>>> +	test_must_fail git config . &&
>>> +	test_must_fail git config .foo &&
>>> +	test_must_fail git config foo. &&
>>> +	test_must_fail git config .foo. &&
>>
>> Not a nit: These tests only show that 'git config' cannot be asked for
>> junk, but they do not show that you cannot insert junk into the config
>> file anymore using 'git config'.
> 
> This change builds on top of "Sanity-check config variable names" which makes
> setting and getting values use the same key checking routine.  For the moment,
> it does not matter if we test the "set" ot "get" path.

The purpose of tests is not to check the implementation, but the observable behavior. Therefore, you want to write a test case to make sure that neither usage is broken by future changes.

-- Hannes
Libor Pechacek· Jan 31, 2011, 13:08 UTC · re: Johannes Sixt · lore

[PATCH v3] 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, adds tests for these cases and also fixes a typo in t5526-fetch-submodules, which was exposed by the new check.

Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
Cc: Johannes Sixt <j.sixt@viscovery.net>
---

Incoporated feedback from Johannes, introduced keylen local variable to improve readability of the code. Applies on top "Sanity-check config variable names".

 config.c                    |   10 ++++++++--
 t/t1300-repo-config.sh      |    4 ++++
 t/t5526-fetch-submodules.sh |    2 +-
 3 files changed, 13 insertions(+), 3 deletions(-)
Show changes to 3 files +13 −3

config.c, t/t1300-repo-config.sh, t/t5526-fetch-submodules.sh

diff --git a/config.c b/config.c
index fde91f5..5eb89a7 100644
--- a/config.c
+++ b/config.c
@@ -1113,6 +1113,7 @@ int git_config_set(const char *key, const char *value)
 int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 {
 	int i, dot, baselen;
+	int keylen = strlen(key);
 	const char *last_dot = strrchr(key, '.');
 
 	/*
@@ -1120,11 +1121,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 || *key == '.') {
 		error("key does not contain a section: %s", key);
 		return -2;
 	}
 
+	if (keylen && key[keylen-1] == '.') {
+		error("key does not contain variable name: %s", key);
+		return -2;
+	}
+
 	baselen = last_dot - key;
 	if (baselen_)
 		*baselen_ = baselen;
@@ -1132,7 +1138,7 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	/*
 	 * Validate the key and while at it, lower case it for matching.
 	 */
-	*store_key = xmalloc(strlen(key) + 1);
+	*store_key = xmalloc(keylen + 1);
 
 	dot = 0;
 	for (i = 0; key[i]; i++) {
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index c3d91d1..53fb822 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
 '
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 884a5e5..7106c6c 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -124,7 +124,7 @@ test_expect_success "--recurse-submodules overrides fetchRecurseSubmodules setti
 	(
 		cd downstream &&
 		git fetch --recurse-submodules >../actual.out 2>../actual.err &&
-		git config -f --unset .gitmodules submodule.submodule.fetchRecurseSubmodules true &&
+		git config -f .gitmodules --unset submodule.submodule.fetchRecurseSubmodules true &&
 		git config --unset submodule.submodule.fetchRecurseSubmodules
 	) &&
 	test_cmp expect.out actual.out &&

-- 
1.7.4.rc3.11.g863f7
Jens Lehmann· Jan 31, 2011, 16:48 UTC · re: Libor Pechacek · lore

Re: [PATCH v3] Disallow empty section and variable names

Am 31.01.2011 14:08, schrieb Libor Pechacek:
Show 10 quoted lines
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index 884a5e5..7106c6c 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -124,7 +124,7 @@ test_expect_success "--recurse-submodules overrides fetchRecurseSubmodules setti
>  	(
>  		cd downstream &&
>  		git fetch --recurse-submodules >../actual.out 2>../actual.err &&
> -		git config -f --unset .gitmodules submodule.submodule.fetchRecurseSubmodules true &&
> +		git config -f .gitmodules --unset submodule.submodule.fetchRecurseSubmodules true &&

This is not quite the right fix (notice the extra "true" value at the end). IMHO this should be fixed separately, patch coming.

Libor Pechacek· Feb 1, 2011, 07:13 UTC · re: Jens Lehmann · lore

[PATCH v4] 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 <lpechacek@suse.cz>
---

Fix in t5526-fetch-submodules.sh was posted separately by Jens Lehmann. Applies on top "Sanity-check config variable names".

 config.c               |   10 ++++++++--
 t/t1300-repo-config.sh |    4 ++++
 2 files changed, 12 insertions(+), 2 deletions(-)
Show changes to 2 files +12 −2

config.c, t/t1300-repo-config.sh

diff --git a/config.c b/config.c
index fde91f5..5eb89a7 100644
--- a/config.c
+++ b/config.c
@@ -1113,6 +1113,7 @@ int git_config_set(const char *key, const char *value)
 int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 {
 	int i, dot, baselen;
+	int keylen = strlen(key);
 	const char *last_dot = strrchr(key, '.');
 
 	/*
@@ -1120,11 +1121,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 || *key == '.') {
 		error("key does not contain a section: %s", key);
 		return -2;
 	}
 
+	if (keylen && key[keylen-1] == '.') {
+		error("key does not contain variable name: %s", key);
+		return -2;
+	}
+
 	baselen = last_dot - key;
 	if (baselen_)
 		*baselen_ = baselen;
@@ -1132,7 +1138,7 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	/*
 	 * Validate the key and while at it, lower case it for matching.
 	 */
-	*store_key = xmalloc(strlen(key) + 1);
+	*store_key = xmalloc(keylen + 1);
 
 	dot = 0;
 	for (i = 0; key[i]; i++) {
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index c3d91d1..53fb822 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
 '

-- 
1.7.4.rc3.11.g863f7
Junio C Hamano· Feb 10, 2011, 22:49 UTC · re: Libor Pechacek · lore

Re: [PATCH v4] Disallow empty section and variable names

Libor Pechacek <lpechacek@suse.cz> writes:
Show 23 quoted lines
> diff --git a/config.c b/config.c
> index fde91f5..5eb89a7 100644
> --- a/config.c
> +++ b/config.c
> @@ -1113,6 +1113,7 @@ int git_config_set(const char *key, const char *value)
>  int git_config_parse_key(const char *key, char **store_key, int *baselen_)
>  {
>  	int i, dot, baselen;
> +	int keylen = strlen(key);
>  	const char *last_dot = strrchr(key, '.');
>  
>  	/*
> @@ -1120,11 +1121,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 || *key == '.') {
>  		error("key does not contain a section: %s", key);
>  		return -2;
>  	}
>  
> +	if (keylen && key[keylen-1] == '.') {
Aren't these the same as saying
	if (!last_dot || last_dot == key) {
		...
        }
	if (!last_dot[1]) { /* we know last_dot is not NULL */
        	...
	}

Again, a minimum fix-up on top of your patch. Thanks.

Show changes to config.c +3 −4
diff --git a/config.c b/config.c
index 75dd629..d5bb862 100644
--- a/config.c
+++ b/config.c
@@ -1113,7 +1113,6 @@ int git_config_set(const char *key, const char *value)
 int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 {
 	int i, dot, baselen;
-	int keylen = strlen(key);
 	const char *last_dot = strrchr(key, '.');
 
 	/*
@@ -1121,12 +1120,12 @@ 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 || *key == '.') {
+	if (last_dot == NULL || last_dot == key) {
 		error("key does not contain a section: %s", key);
 		return -2;
 	}
 
-	if (keylen && key[keylen-1] == '.') {
+	if (!last_dot[1]) {
 		error("key does not contain variable name: %s", key);
 		return -2;
 	}
@@ -1138,7 +1137,7 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	/*
 	 * Validate the key and while at it, lower case it for matching.
 	 */
-	*store_key = xmalloc(keylen + 1);
+	*store_key = xmalloc(strlen(key) + 1);
 
 	dot = 0;
 	for (i = 0; key[i]; i++) {

← back to recent threads