[PATCH v3] 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-quote filenames which escapes control characters and invalid bytes to printable text. That avoids this bug from being triggered; however, with core.quotePath=false, most characters are no longer escaped (though some control characters still are) and 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 several C1 control characters (repeats of 0xC2 0x9F, causing the loop to read past the end of the string). The second test reliably catches the out-of-bounds read when run under ASan, though it may pass silently without sanitizers.

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 v2:
    
     * Fixed the filename in the final test such that it will trigger the
       out-of-bounds read under ASan, and updated the commit message to
       point out that ASan is needed to notice the out-of-bounds read.
    
    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-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2093/newren/fix/diffstat-utf8-loop-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2093

Range-diff vs v2:

 1:  4a72647ce2 ! 1:  4a3126720b diff: fix out-of-bounds reads and NULL deref in diffstat UTF-8 truncation
     @@ Commit message
              consume more characters than the string contains, reading past
              the trailing NUL.
      
     -    By default, fill_print_name() will C-quotes filenames which escapes
     +    By default, fill_print_name() will C-quote 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.
     +    most characters are no longer escaped (though some control characters
     +    still are) and 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).
     +    byte (invalid UTF-8 lead byte, triggers NULL deref) and one with
     +    several C1 control characters (repeats of 0xC2 0x9F, causing
     +    the loop to read past the end of the string).  The second test
     +    reliably catches the out-of-bounds read when run under ASan, though
     +    it may pass silently without sanitizers.
      
          Fix both issues by introducing utf8_ish_width(), a thin wrapper
          around utf8_width() that guarantees the pointer always advances and
     @@ t/t4052-stat-output.sh: test_expect_success 'diffstat where line_prefix contains
      +	test_grep "| 0" output
      +'
      +
     -+test_expect_success FUNNYNAMES 'diffstat truncation with control chars does not crash' '
     -+	FNAME=$(printf "aaa-\x01-aaa") &&
     ++test_expect_success FUNNYNAMES 'diffstat truncation with control chars does not read out of bounds' '
     ++	FNAME=$(printf "aaa-\302\237\302\237\302\237-aaa") &&
      +	git commit --allow-empty -m setup &&
      +	>$FNAME &&
      +	git add -- $FNAME &&

 diff.c                 | 26 ++++++++++++++++++++++++--
 t/t4052-stat-output.sh | 25 +++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 2 deletions(-)