diff: fix out-of-bounds reads and NULL deref in diffstat UTF-8 truncation by newren · Pull Request #2093 · gitgitgadget/git

newren · 2026-04-17T00:32:17Z

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.)

cc: LorenzoPegorari lorenzo.pegorari2002@gmail.com
cc: Elijah Newren newren@gmail.com

newren · 2026-04-17T16:24:49Z

/submit

gitgitgadget · 2026-04-17T16:26:21Z

Submitted as pull.2093.git.1776443163041.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2093/newren/fix/diffstat-utf8-loop-v1

To fetch this version to local tag pr-2093/newren/fix/diffstat-utf8-loop-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2093/newren/fix/diffstat-utf8-loop-v1

gitgitgadget · 2026-04-17T20:04:47Z

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> f85b49f3d4a (diff: improve scaling of filenames in diffstat to handle
> UTF-8 chars, 2024-10-27) introduced a loop in show_stats() that calls
> utf8_width() repeatedly to skip leading characters until the displayed
> width fits.

A tangent, but I get a datestamp for the same f85b49f3 (diff:
improve scaling of filenames in diffstat to handle UTF-8 chars,
2026-01-16) that is different from what you showed above.  Did you
find a bug in "git show -s --pretty=reference"?

> diff --git a/diff.c b/diff.c
> index 397e38b41c..7b27241733 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3093,8 +3093,17 @@ 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;
> +				}

IOW, we punt on "scaling" and instead use the full string?  I was
wondering if we can punt on only this segment by replacing this
segment with just "..." and resync at the next slash.

> +				if (w < 0)  /* control character */
> +					break;

When we have a control characer, we instead chomp immediately before
that byte, which sounds good.  But then wouldn't the loop that found
an Invalid UTF-8 sequence in the middle of a name want to do the
same, i.e., take the good bits found so far and chomp at the broken
byte?

> +				name_len -= w;
> +			}
>  
>  			slash = strchr(name, '/');
>  			if (slash)

Thanks.

…tion f85b49f (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>

gitgitgadget · 2026-04-17T22:39:45Z

Elijah Newren wrote on the Git mailing list (how to reply to this email):

On Fri, Apr 17, 2026 at 12:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > f85b49f3d4a (diff: improve scaling of filenames in diffstat to handle
> > UTF-8 chars, 2024-10-27) introduced a loop in show_stats() that calls
> > utf8_width() repeatedly to skip leading characters until the displayed
> > width fits.
>
> A tangent, but I get a datestamp for the same f85b49f3 (diff:
> improve scaling of filenames in diffstat to handle UTF-8 chars,
> 2026-01-16) that is different from what you showed above.  Did you
> find a bug in "git show -s --pretty=reference"?

Hmm, indeed I get 2026-01-16 as well; I'm not sure what happened there.

> > diff --git a/diff.c b/diff.c
> > index 397e38b41c..7b27241733 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -3093,8 +3093,17 @@ 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;
> > +                             }
>
> IOW, we punt on "scaling" and instead use the full string?  I was
> wondering if we can punt on only this segment by replacing this
> segment with just "..." and resync at the next slash.

Good point.  Alternatively, perhaps I could just add a wrapper around
utf8_width() which never sets name to NULL and never returns a
negative value, and then use the original loop as-is other than
calling the new function?

>
> > +                             if (w < 0)  /* control character */
> > +                                     break;
>
> When we have a control characer, we instead chomp immediately before
> that byte, which sounds good.  But then wouldn't the loop that found
> an Invalid UTF-8 sequence in the middle of a name want to do the
> same, i.e., take the good bits found so far and chomp at the broken
> byte?

Makes sense, though I think my simpler alternative might be easier.
I'll send in a re-roll.

>
> > +                             name_len -= w;
> > +                     }
> >
> >                       slash = strchr(name, '/');
> >                       if (slash)
>
> Thanks.

gitgitgadget · 2026-04-17T22:39:47Z

User Elijah Newren <newren@gmail.com> has been added to the cc: list.

gitgitgadget · 2026-04-17T22:39:55Z

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

Elijah Newren <newren@gmail.com> writes:

> Makes sense, though I think my simpler alternative might be easier.
> I'll send in a re-roll.

As long as "an invalid UTF-8" and "a control character" behaves more
or less the same (i.e., "eek, we cannot measure the width of the
UTF-8 character at this byte position, so let's do X as a fallback",
where X is the same regardless of the exact reason why we cannot
measure the width), I'll be happy.  If we see a slash after the
problematic position, advancing to that slash might be the simplest,
as that is in line with how the code works when there is no such
problem, but we also need to be prepared for a filename whose last
component is sufficiently long that we see no such slash after the
problematic byte.

newren · 2026-04-17T22:44:11Z

/submit

gitgitgadget · 2026-04-17T22:45:22Z

Submitted as pull.2093.v2.git.1776465910538.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2093/newren/fix/diffstat-utf8-loop-v2

To fetch this version to local tag pr-2093/newren/fix/diffstat-utf8-loop-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2093/newren/fix/diffstat-utf8-loop-v2

gitgitgadget · 2026-04-18T05:18:21Z

This branch is now known as en/diffstat-utf8-truncation-fix.

newren force-pushed the fix/diffstat-utf8-loop branch 3 times, most recently from c0e31d0 to fcd44d6 Compare April 17, 2026 12:23

newren force-pushed the fix/diffstat-utf8-loop branch from fcd44d6 to 4a72647 Compare April 17, 2026 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

diff: fix out-of-bounds reads and NULL deref in diffstat UTF-8 truncation#2093

diff: fix out-of-bounds reads and NULL deref in diffstat UTF-8 truncation#2093
newren wants to merge 1 commit intogitgitgadget:masterfrom
newren:fix/diffstat-utf8-loop

newren commented Apr 17, 2026 •

edited

Loading

Uh oh!

newren commented Apr 17, 2026

Uh oh!

gitgitgadget bot commented Apr 17, 2026

Uh oh!

gitgitgadget bot commented Apr 17, 2026

Uh oh!

gitgitgadget bot commented Apr 17, 2026

Uh oh!

gitgitgadget bot commented Apr 17, 2026

Uh oh!

gitgitgadget bot commented Apr 17, 2026

Uh oh!

newren commented Apr 17, 2026

Uh oh!

gitgitgadget bot commented Apr 17, 2026

Uh oh!

gitgitgadget bot commented Apr 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

newren commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

newren commented Apr 17, 2026

Uh oh!

gitgitgadget bot commented Apr 17, 2026

Uh oh!

gitgitgadget bot commented Apr 17, 2026

Uh oh!

gitgitgadget bot commented Apr 17, 2026

Uh oh!

gitgitgadget bot commented Apr 17, 2026

Uh oh!

gitgitgadget bot commented Apr 17, 2026

Uh oh!

newren commented Apr 17, 2026

Uh oh!

gitgitgadget bot commented Apr 17, 2026

Uh oh!

gitgitgadget bot commented Apr 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

newren commented Apr 17, 2026 •

edited

Loading