[PATCH v2] diff: fix out-of-bounds reads and NULL deref in diffstat UTF-8 truncation
From: Elijah Newren <newren@gmail.com>
f85b49f3d4a (diff: improve scaling of filenames in diffstat to handle UTF-8 chars, 2026-01-16) introduced a loop in show_stats() that calls utf8_width() repeatedly to skip leading characters until the displayed width fits. However, utf8_width() can return problematic values:
- For invalid UTF-8 sequences, pick_one_utf8_char() sets the name
pointer to NULL and utf8_width() returns 0. Since name_len does
not change, the loop iterates once more and pick_one_utf8_char()
dereferences the NULL pointer, crashing. - For control characters, utf8_width() returns -1, so name_len
grows when it is expected to shrink. This can cause the loop to
consume more characters than the string contains, reading past
the trailing NUL.By default, fill_print_name() will C-quotes filenames which escapes control characters and invalid bytes to printable text. That avoids this bug from being triggered; however, with core.quotePath=false, raw bytes can reach this code.
Add tests exercising both failure modes with core.quotePath=false and a narrow --stat-name-width to force truncation: one with a bare 0xC0 byte (invalid UTF-8 lead byte, triggers NULL deref) and one with a 0x01 byte (control character, causes the loop to read past the end of the string).
Fix both issues by introducing utf8_ish_width(), a thin wrapper around utf8_width() that guarantees the pointer always advances and the returned width is never negative:
- On invalid UTF-8 it restores the pointer, advances by one byte,
and returns width 1 (matching the strlen()-based fallback used
by utf8_strwidth()).
- On a control character it returns 0 (matching utf8_strnwidth()
which skips them).Also add a "&& *name" guard to the while-loop condition so it terminates at end-of-string even when utf8_strwidth()'s strlen() fallback causes name_len to exceed the sum of per-character widths.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
diff: fix out-of-bounds reads and NULL deref in diffstat UTF-8
truncation
Changes since v1:
* Simplified the loop to almost what we had before via a wrapper
function that always succeeds in advancing the string and never
returns a negative width. (Which, as a consequence, treats invalid
UTF-8 and control characters the roughly the same, unlike v1.)Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2093%2Fnewren%2Ffix%2Fdiffstat-utf8-loop-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2093/newren/fix/diffstat-utf8-loop-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2093
Range-diff vs v1:
1: fcd44d6cf8 ! 1: 4a72647ce2 diff: fix out-of-bounds reads and NULL deref in diffstat UTF-8 truncation
@@ Commit message
diff: fix out-of-bounds reads and NULL deref in diffstat UTF-8 truncation
f85b49f3d4a (diff: improve scaling of filenames in diffstat to handle
- UTF-8 chars, 2024-10-27) introduced a loop in show_stats() that calls
+ UTF-8 chars, 2026-01-16) introduced a loop in show_stats() that calls
utf8_width() repeatedly to skip leading characters until the displayed
width fits. However, utf8_width() can return problematic values:
@@ Commit message
0x01 byte (control character, causes the loop to read past the end
of the string).
- Fix the bug by:
- - Adding a *name check to terminate the loop at end-of-string
- - Detecting the NULL pointer from invalid UTF-8 and falling back to
- showing the full untruncated name
- - Breaking on negative width (control characters)
+ Fix both issues by introducing utf8_ish_width(), a thin wrapper
+ around utf8_width() that guarantees the pointer always advances and
+ the returned width is never negative:
+
+ - On invalid UTF-8 it restores the pointer, advances by one byte,
+ and returns width 1 (matching the strlen()-based fallback used
+ by utf8_strwidth()).
+ - On a control character it returns 0 (matching utf8_strnwidth()
+ which skips them).
+
+ Also add a "&& *name" guard to the while-loop condition so it
+ terminates at end-of-string even when utf8_strwidth()'s strlen()
+ fallback causes name_len to exceed the sum of per-character widths.
Signed-off-by: Elijah Newren <newren@gmail.com>
## diff.c ##
+@@ diff.c: void print_stat_summary(FILE *fp, int files,
+ print_stat_summary_inserts_deletes(&o, files, insertions, deletions);
+ }
+
++/*
++ * Like utf8_width(), but guaranteed safe for use in loops that subtract
++ * per-character widths:
++ *
++ * - utf8_width() sets *start to NULL on invalid UTF-8 and returns 0;
++ * we restore the pointer and advance by one byte, returning width 1
++ * (matching the strlen()-based fallback in utf8_strwidth()).
++ *
++ * - utf8_width() returns -1 for control characters; we return 0
++ * (matching utf8_strnwidth() which skips them).
++ */
++static int utf8_ish_width(const char **start)
++{
++ const char *old = *start;
++ int w = utf8_width(start, NULL);
++ if (!*start) {
++ *start = old + 1;
++ return 1;
++ }
++ return (w < 0) ? 0 : w;
++}
++
+ static void show_stats(struct diffstat_t *data, struct diff_options *options)
+ {
+ int i, len, add, del, adds = 0, dels = 0;
@@ diff.c: static void show_stats(struct diffstat_t *data, struct diff_options *options)
if (len < 0)
len = 0;
- while (name_len > len)
- name_len -= utf8_width((const char**)&name, NULL);
-+ while (name_len > len && *name) {
-+ int w = utf8_width((const char **)&name, NULL);
-+ if (!name) { /* Invalid UTF-8 */
-+ name = file->print_name;
-+ name_len = utf8_strwidth(name);
-+ break;
-+ }
-+ if (w < 0) /* control character */
-+ break;
-+ name_len -= w;
-+ }
++ while (name_len > len && *name)
++ name_len -= utf8_ish_width((const char**)&name);
slash = strchr(name, '/');
if (slash) diff.c | 26 ++++++++++++++++++++++++--
t/t4052-stat-output.sh | 25 +++++++++++++++++++++++++
2 files changed, 49 insertions(+), 2 deletions(-)
Show changes to 2 files +49 −2
diff.c, t/t4052-stat-output.sh
diff --git a/diff.c b/diff.c
index 397e38b41c..1a3b19f71f 100644
--- a/diff.c
+++ b/diff.c
@@ -2927,6 +2927,28 @@ void print_stat_summary(FILE *fp, int files,
print_stat_summary_inserts_deletes(&o, files, insertions, deletions);
}
+/*
+ * Like utf8_width(), but guaranteed safe for use in loops that subtract
+ * per-character widths:
+ *
+ * - utf8_width() sets *start to NULL on invalid UTF-8 and returns 0;
+ * we restore the pointer and advance by one byte, returning width 1
+ * (matching the strlen()-based fallback in utf8_strwidth()).
+ *
+ * - utf8_width() returns -1 for control characters; we return 0
+ * (matching utf8_strnwidth() which skips them).
+ */
+static int utf8_ish_width(const char **start)
+{
+ const char *old = *start;
+ int w = utf8_width(start, NULL);
+ if (!*start) {
+ *start = old + 1;
+ return 1;
+ }
+ return (w < 0) ? 0 : w;
+}
+
static void show_stats(struct diffstat_t *data, struct diff_options *options)
{
int i, len, add, del, adds = 0, dels = 0;
@@ -3093,8 +3115,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
if (len < 0)
len = 0;
- while (name_len > len)
- name_len -= utf8_width((const char**)&name, NULL);
+ while (name_len > len && *name)
+ name_len -= utf8_ish_width((const char**)&name);
slash = strchr(name, '/');
if (slash)
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 7c749062e2..84c53c1a51 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -445,4 +445,29 @@ test_expect_success 'diffstat where line_prefix contains ANSI escape codes is co
test_grep "<RED>|<RESET> ${FILENAME_TRIMMED} | 0" out
'
+test_expect_success 'diffstat truncation with invalid UTF-8 does not crash' '
+ empty_blob=$(git hash-object -w --stdin </dev/null) &&
+ printf "100644 blob $empty_blob\taaa-\300-aaa\n" |
+ git mktree >tree_file &&
+ tree=$(cat tree_file) &&
+ empty_tree=$(git mktree </dev/null) &&
+ c1=$(git commit-tree -m before $empty_tree) &&
+ c2=$(git commit-tree -m after -p $c1 $tree) &&
+ git -c core.quotepath=false diff --stat --stat-name-width=5 $c1..$c2 >output &&
+ test_grep "| 0" output
+'
+
+test_expect_success FUNNYNAMES 'diffstat truncation with control chars does not crash' '
+ FNAME=$(printf "aaa-\x01-aaa") &&
+ git commit --allow-empty -m setup &&
+ >$FNAME &&
+ git add -- $FNAME &&
+ git commit -m "add file with control char name" &&
+ git -c core.quotepath=false diff --stat --stat-name-width=5 HEAD~1..HEAD >output &&
+ test_grep "| 0" output &&
+ rm -- $FNAME &&
+ git rm -- $FNAME &&
+ git commit -m "remove test file"
+'
+
test_done
base-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199