From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 1 Dec 2022 14:47:10 +0000 (+0100)
Subject: utf8: fix checking for glyph width in `strbuf_utf8_replace()`
X-Git-Tag: v2.30.7~2^2~2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=81c2d4c3a5ba0e6ab8c348708441fed170e63a82;p=thirdparty%2Fgit.git

utf8: fix checking for glyph width in `strbuf_utf8_replace()`

In `strbuf_utf8_replace()`, we call `utf8_width()` to compute the width
of the current glyph. If the glyph is a control character though it can
be that `utf8_width()` returns `-1`, but because we assign this value to
a `size_t` the conversion will cause us to underflow. This bug can
easily be triggered with the following command:

    $ git log --pretty='format:xxx%<|(1,trunc)%x10'

>From all I can see though this seems to be a benign underflow that has
no security-related consequences.

Fix the bug by using an `int` instead. When we see a control character,
we now copy it into the target buffer but don't advance the current
width of the string.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index aac9e4ce6c..5c5b56596e 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -905,6 +905,13 @@ test_expect_success 'log --pretty with padding and preceding control chars' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log --pretty truncation with control chars' '
+	test_commit "$(printf "\20\20\20\20xxxx")" file contents commit-with-control-chars &&
+	printf "\20\20\20\20x.." >expect &&
+	git log -1 --pretty="format:%<(3,trunc)%s" commit-with-control-chars >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'log --pretty with huge commit message' '
 	# We only assert that this command does not crash. This needs to be
 	# executed with the address sanitizer to demonstrate failure.
diff --git a/utf8.c b/utf8.c
index 30c7787cfa..077daf4b20 100644
--- a/utf8.c
+++ b/utf8.c
@@ -377,6 +377,7 @@ void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width,
 	dst = sb_dst.buf;
 
 	while (src < end) {
+		int glyph_width;
 		char *old;
 		size_t n;
 
@@ -390,21 +391,29 @@ void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width,
 			break;
 
 		old = src;
-		n = utf8_width((const char**)&src, NULL);
-		if (!src) 	/* broken utf-8, do nothing */
+		glyph_width = utf8_width((const char**)&src, NULL);
+		if (!src) /* broken utf-8, do nothing */
 			goto out;
-		if (n && w >= pos && w < pos + width) {
+
+		/*
+		 * In case we see a control character we copy it into the
+		 * buffer, but don't add it to the width.
+		 */
+		if (glyph_width < 0)
+			glyph_width = 0;
+
+		if (glyph_width && w >= pos && w < pos + width) {
 			if (subst) {
 				memcpy(dst, subst, subst_len);
 				dst += subst_len;
 				subst = NULL;
 			}
-			w += n;
+			w += glyph_width;
 			continue;
 		}
 		memcpy(dst, old, src - old);
 		dst += src - old;
-		w += n;
+		w += glyph_width;
 	}
 	strbuf_setlen(&sb_dst, dst - sb_dst.buf);
 	strbuf_swap(sb_src, &sb_dst);