git config problem with strange config files

git config problem with strange config files

[Peter Oberndorfer]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

Hi,

In case of a strange .git/config file "git config key value" can create a 
bogus config entry, where one old value is wrong and the new value resides in 
the wrong section until the config file is fixed up by hand

The attached test config has a trailing tab in the last line
and no newline after that.
When git config is run the new section name is placed directly after the 
trailing tab, which corrupts the old value.

To reproduce  the error do the following:

mkdir git_test_config && cd git_test_config
git init
<copy the attached config file to .git/config>
git config section2.key bar
git config --get section2.key
-> returns nothing
git config --get section.key
-> foo [section2]
-> error: More than one value for the key section.key: bar

i tried to make a test out of this but i failed to create a config file 
without a trailing newline using here documents :-(

Tested using v1.5.4.rc2 and v1.5.4-rc2-6-gab11903

Greetings Peter

[-- Attachment #2: config --]
[-- Type: text/plain, Size: 21 bytes --]

[section]
	key = foo	

[-- Attachment #3: config_bogus --]
[-- Type: text/plain, Size: 43 bytes --]

[section]
	key = foo	[section2]
	key = bar

Re: git config problem with strange config files

[Jeff King]

On Mon, Dec 31, 2007 at 06:47:41PM +0100, Peter Oberndorfer wrote:

> In case of a strange .git/config file "git config key value" can
> create a bogus config entry, where one old value is wrong and the new
> value resides in the wrong section until the config file is fixed up
> by hand

This patch fixes the test case you gave.

Junio, even though such a config file should be rare, I think this is
v1.5.4 material. But there is one tricky thing which I will point out in
a followup mail.

-- >8 --
config: handle lack of newline at end of file better

The config parsing routines use the static global
'config_file' to store the FILE* pointing to the current
config file being parsed. The function get_next_char()
automatically converts an EOF on this file to a newline for
the convenience of its callers, and it sets config_file to
NULL to indicate that EOF was reached.

This throws away useful information, though, since some
routines want to call ftell on 'config_file' to find out
exactly _where_ the routine ended. In the case of a key
ending at EOF boundary, we ended up segfaulting in some
cases (changing that key or adding another key in its
section), or failing to provide the necessary newline
(adding a new section).

This patch adds a new flag to indicate EOF and uses that
instead of setting config_file to NULL. It also makes sure
to add newlines where necessary for truncated input. All
three included tests fail without the patch.
---
 config.c                |   14 +++++++++-----
 t/t1303-wacky-config.sh |   37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 5 deletions(-)
 create mode 100755 t/t1303-wacky-config.sh

diff --git a/config.c b/config.c
index fa56c70..7ef8b75 100644
--- a/config.c
+++ b/config.c
@@ -13,6 +13,7 @@
 static FILE *config_file;
 static const char *config_file_name;
 static int config_linenr;
+static int config_file_eof;
 static int zlib_compression_seen;
 
 static int get_next_char(void)
@@ -34,7 +35,7 @@ static int get_next_char(void)
 		if (c == '\n')
 			config_linenr++;
 		if (c == EOF) {
-			config_file = NULL;
+			config_file_eof = 1;
 			c = '\n';
 		}
 	}
@@ -118,7 +119,7 @@ static int get_value(config_fn_t fn, char *name, unsigned int len)
 	/* Get the full name */
 	for (;;) {
 		c = get_next_char();
-		if (c == EOF)
+		if (config_file_eof)
 			break;
 		if (!iskeychar(c))
 			break;
@@ -182,7 +183,7 @@ static int get_base_var(char *name)
 
 	for (;;) {
 		int c = get_next_char();
-		if (c == EOF)
+		if (config_file_eof)
 			return -1;
 		if (c == ']')
 			return baselen;
@@ -205,8 +206,7 @@ static int git_parse_file(config_fn_t fn)
 	for (;;) {
 		int c = get_next_char();
 		if (c == '\n') {
-			/* EOF? */
-			if (!config_file)
+			if (config_file_eof)
 				return 0;
 			comment = 0;
 			continue;
@@ -469,6 +469,7 @@ int git_config_from_file(config_fn_t fn, const char *filename)
 		config_file = f;
 		config_file_name = filename;
 		config_linenr = 1;
+		config_file_eof = 0;
 		ret = git_parse_file(fn);
 		fclose(f);
 		config_file_name = NULL;
@@ -918,6 +919,9 @@ int git_config_set_multivar(const char* key, const char* value,
 					contents, contents_sz,
 					store.offset[i]-2, &new_line);
 
+			if (copy_end > 0 && contents[copy_end-1] != '\n')
+				new_line = 1;
+
 			/* write the first part of the config */
 			if (copy_end > copy_begin) {
 				if (write_in_full(fd, contents + copy_begin,
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
new file mode 100755
index 0000000..99985dc
--- /dev/null
+++ b/t/t1303-wacky-config.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='Test wacky input to git config'
+. ./test-lib.sh
+
+setup() {
+	(printf "[section]\n" &&
+	printf "  key = foo") >.git/config
+}
+
+check() {
+	echo "$2" >expected
+	git config --get "$1" >actual
+	git diff actual expected
+}
+
+test_expect_success 'modify same key' '
+	setup &&
+	git config section.key bar &&
+	check section.key bar
+'
+
+test_expect_success 'add key in same section' '
+	setup &&
+	git config section.other bar &&
+	check section.key foo &&
+	check section.other bar
+'
+
+test_expect_success 'add key in different section' '
+	setup &&
+	git config section2.key bar &&
+	check section.key foo &&
+	check section2.key bar
+'
+
+test_done
-- 
1.5.4.rc2.1103.g1042-dirty

Re: git config problem with strange config files

[Jeff King]

On Tue, Jan 01, 2008 at 01:17:34AM -0500, Jeff King wrote:

> @@ -118,7 +119,7 @@ static int get_value(config_fn_t fn, char *name, unsigned int len)
>  	/* Get the full name */
>  	for (;;) {
>  		c = get_next_char();
> -		if (c == EOF)
> +		if (config_file_eof)
>  			break;
>  		if (!iskeychar(c))
>  			break;
> @@ -182,7 +183,7 @@ static int get_base_var(char *name)
>  
>  	for (;;) {
>  		int c = get_next_char();
> -		if (c == EOF)
> +		if (config_file_eof)
>  			return -1;
>  		if (c == ']')
>  			return baselen;

In these two hunks, we now check the eof flag correctly, which follows
through on the intent of the original code. But if you look carefully at
get_next_char(), it _never_ returns EOF in the first place; it replaces
it with '\n' and sets an external flag. So this 'if' branch was never
reached, but happened to work because the returned newline triggers a
similar effect.

By actually looking at the flag, I am implementing the original intent,
but it is a potential behavior change (I don't _think_ it should matter,
and it passes all of the config tests, but I haven't looked carefully at
all the ramifications). So it might be simpler yet to just eliminate
that conditional return.

-Peff

Re: git config problem with strange config files

[Peter Oberndorfer]

On Dienstag 01 Januar 2008, Jeff King wrote:
> On Mon, Dec 31, 2007 at 06:47:41PM +0100, Peter Oberndorfer wrote:
> 
> > In case of a strange .git/config file "git config key value" can
> > create a bogus config entry, where one old value is wrong and the new
> > value resides in the wrong section until the config file is fixed up
> > by hand
> 
> This patch fixes the test case you gave.
> 
> Junio, even though such a config file should be rare, I think this is
> v1.5.4 material. But there is one tricky thing which I will point out in
> a followup mail.
> 
Thank you for the patch

I would also guess this is relative rare, but IIRC somebody posted a patch to 
handle a bogus config with a better message,
and the breakage example in the mail looked similar to mine.

How i came up with this bogus config is relative simple.
I wanted to add a new remote so i copied the already existing remote and 
branch sections down to the last line and somehow also added a trailing tab.

Then i did a stg init which uses git repo-config to add its
stackformatversion = 2 setting

Greetings Peter

Re: git config problem with strange config files

[Junio C Hamano]

I agree with your analysis, and thanks for the patch.