[PATCH] config: read the bytes of a blob as unsigned characters

[PATCH] config: read the bytes of a blob as unsigned characters

[Olof Fairbrook]

"git config --blob" and the other readers of config from memory get
each byte from config_buf_fgetc(), which returns it as a plain "char".
Where "char" is signed, as on x86, a byte 0xff comes back as -1, which
is EOF, and any byte from 0x80 up as a negative number.  So the parser
stops at a byte 0xff and silently ignores the rest of the blob:

  $ printf '# caf\377\n[some]\n\tkey = value\n' >config
  $ git config --blob=$(git hash-object -w config) --list
  $

and a blob that starts with a UTF-8 byte order mark is a "bad config
line 1", because the code that skips the mark compares with unsigned
bytes.  A config file has neither problem, as getc() returns unsigned
bytes.  ".gitmodules" read from a commit goes through the same reader.

This has been so since config_buf_fgetc() was added in 1bc888193e
(teach config --blob option to parse config from database,
2013-07-12).

Return the byte as an unsigned char, as getc() does.
config_buf_ungetc() checks that it is given back the byte it read; it
now compares with the byte as an unsigned char too, or a CR followed
by such a byte would hit its BUG().

Signed-off-by: Olof Fairbrook <olof.fairbrook@gmail.com>
---
 config.c               |  4 ++--
 t/t1307-config-blob.sh | 21 +++++++++++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index d9019e7e6c..fbe873274e 100644
--- a/config.c
+++ b/config.c
@@ -92,7 +92,7 @@ static long config_file_ftell(struct config_source *conf)
 static int config_buf_fgetc(struct config_source *conf)
 {
 	if (conf->u.buf.pos < conf->u.buf.len)
-		return conf->u.buf.buf[conf->u.buf.pos++];
+		return (unsigned char)conf->u.buf.buf[conf->u.buf.pos++];
 
 	return EOF;
 }
@@ -101,7 +101,7 @@ static int config_buf_ungetc(int c, struct config_source *conf)
 {
 	if (conf->u.buf.pos > 0) {
 		conf->u.buf.pos--;
-		if (conf->u.buf.buf[conf->u.buf.pos] != c)
+		if ((unsigned char)conf->u.buf.buf[conf->u.buf.pos] != c)
 			BUG("config_buf can only ungetc the same character");
 		return c;
 	}
diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh
index 5cb546dd00..e2c0883350 100755
--- a/t/t1307-config-blob.sh
+++ b/t/t1307-config-blob.sh
@@ -72,6 +72,27 @@ test_expect_success 'can parse blob ending with CR' '
 	test_cmp expect actual
 '
 
+test_expect_success 'a byte 0xff does not end a blob' '
+	test_commit --printf FF config "# caf\\377\\n[some]\\n\\tkey = value\\n" &&
+	echo value >expect &&
+	git config --blob=HEAD:config some.key >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'a byte 0xff after a CR in a blob' '
+	test_commit --printf CRFF config "# a\\r\\377\\n[some]\\n\\tkey = value\\n" &&
+	echo value >expect &&
+	git config --blob=HEAD:config some.key >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'can parse blob starting with a byte order mark' '
+	test_commit --printf BOM config "\\357\\273\\277[some]\\n\\tkey = value\\n" &&
+	echo value >expect &&
+	git config --blob=HEAD:config some.key >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'config --blob outside of a repository is an error' '
 	nongit test_must_fail git config --blob=foo --list
 '

base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
-- 
2.43.0

Re: [PATCH] config: read the bytes of a blob as unsigned characters

[Junio C Hamano]

Olof Fairbrook <olof.fairbrook@gmail.com> writes:

> "git config --blob" and the other readers of config from memory get
> each byte from config_buf_fgetc(), which returns it as a plain "char".
> Where "char" is signed, as on x86, a byte 0xff comes back as -1, which
> is EOF, and any byte from 0x80 up as a negative number.  So the parser
> stops at a byte 0xff and silently ignores the rest of the blob:
>
>   $ printf '# caf\377\n[some]\n\tkey = value\n' >config
>   $ git config --blob=$(git hash-object -w config) --list
>   $
>
> and a blob that starts with a UTF-8 byte order mark is a "bad config
> line 1", because the code that skips the mark compares with unsigned
> bytes.  A config file has neither problem, as getc() returns unsigned
> bytes.  ".gitmodules" read from a commit goes through the same reader.

Nicely explained, even though it is a tad overly verbose side
(especially given that the example code is something we can see in
the added test easily).

> This has been so since config_buf_fgetc() was added in 1bc888193e
> (teach config --blob option to parse config from database,
> 2013-07-12).
>
> Return the byte as an unsigned char, as getc() does.
> config_buf_ungetc() checks that it is given back the byte it read; it
> now compares with the byte as an unsigned char too, or a CR followed
> by such a byte would hit its BUG().

Very nice.  Will queue as-is.

> diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh
> index 5cb546dd00..e2c0883350 100755
> --- a/t/t1307-config-blob.sh
> +++ b/t/t1307-config-blob.sh
> @@ -72,6 +72,27 @@ test_expect_success 'can parse blob ending with CR' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'a byte 0xff does not end a blob' '
> +	test_commit --printf FF config "# caf\\377\\n[some]\\n\\tkey = value\\n" &&
> +	echo value >expect &&
> +	git config --blob=HEAD:config some.key >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'a byte 0xff after a CR in a blob' '
> +	test_commit --printf CRFF config "# a\\r\\377\\n[some]\\n\\tkey = value\\n" &&
> +	echo value >expect &&
> +	git config --blob=HEAD:config some.key >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'can parse blob starting with a byte order mark' '
> +	test_commit --printf BOM config "\\357\\273\\277[some]\\n\\tkey = value\\n" &&
> +	echo value >expect &&
> +	git config --blob=HEAD:config some.key >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'config --blob outside of a repository is an error' '
>  	nongit test_must_fail git config --blob=foo --list
>  '
>
> base-commit: a018953688f1b10bddf91bff8747068f5f4746a4