# [PATCH] Disallow empty section and variable names

9 messages from 2011-01-27 to 2011-02-10. Participants: Libor Pechacek, Johannes Sixt, Jens Lehmann, Junio C Hamano.
Thread: https://gitlist.dev/t/26349

## Libor Pechacek, 2011-01-27 14:52

Subject: [PATCH] Disallow empty section and variable names
Message-ID: <20110127145253.GD6312@fm.suse.cz>
URL: https://gitlist.dev/e/20110127145253.GD6312%40fm.suse.cz

```
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(-)

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, 2011-01-30 20:34

Subject: [PATCH v2] Disallow empty section and variable names
Message-ID: <20110130203445.GA9689@fm.suse.cz>
URL: https://gitlist.dev/e/20110130203445.GA9689%40fm.suse.cz
In-Reply-To: <20110127145253.GD6312@fm.suse.cz>

```
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(-)

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, 2011-01-31 07:48

Subject: Re: [PATCH v2] Disallow empty section and variable names
Message-ID: <4D46694F.5070208@viscovery.net>
URL: https://gitlist.dev/e/4D46694F.5070208%40viscovery.net
In-Reply-To: <20110130203445.GA9689@fm.suse.cz>

```
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

> +	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, 2011-01-31 09:17

Subject: Re: [PATCH v2] Disallow empty section and variable names
Message-ID: <20110131091728.GB24297@fm.suse.cz>
URL: https://gitlist.dev/e/20110131091728.GB24297%40fm.suse.cz
In-Reply-To: <4D46694F.5070208@viscovery.net>

```
On Mon 31-01-11 08:48:31, Johannes Sixt wrote:
> 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.

> > +	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, 2011-01-31 09:29

Subject: Re: [PATCH v2] Disallow empty section and variable names
Message-ID: <4D468109.8020409@viscovery.net>
URL: https://gitlist.dev/e/4D468109.8020409%40viscovery.net
In-Reply-To: <20110131091728.GB24297@fm.suse.cz>

```
Am 1/31/2011 10:17, schrieb Libor Pechacek:
> 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, 2011-01-31 13:08

Subject: [PATCH v3] Disallow empty section and variable names
Message-ID: <20110131130855.GC24297@fm.suse.cz>
URL: https://gitlist.dev/e/20110131130855.GC24297%40fm.suse.cz
In-Reply-To: <4D468109.8020409@viscovery.net>

```
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(-)

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, 2011-01-31 16:48

Subject: Re: [PATCH v3] Disallow empty section and variable names
Message-ID: <4D46E7E7.4010805@web.de>
URL: https://gitlist.dev/e/4D46E7E7.4010805%40web.de
In-Reply-To: <20110131130855.GC24297@fm.suse.cz>

```
Am 31.01.2011 14:08, schrieb Libor Pechacek:
> 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, 2011-02-01 07:13

Subject: [PATCH v4] Disallow empty section and variable names
Message-ID: <20110201071347.GH24297@fm.suse.cz>
URL: https://gitlist.dev/e/20110201071347.GH24297%40fm.suse.cz
In-Reply-To: <4D46E7E7.4010805@web.de>

```
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(-)

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, 2011-02-10 22:49

Subject: Re: [PATCH v4] Disallow empty section and variable names
Message-ID: <7v1v3f33pz.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7v1v3f33pz.fsf%40alter.siamese.dyndns.org
In-Reply-To: <20110201071347.GH24297@fm.suse.cz>

```
Libor Pechacek <lpechacek@suse.cz> writes:

> 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.


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++) {

```