From: Elijah Newren <newren@gmail.com>
Date: Fri, 17 Apr 2026 22:00:11 GMT
Subject: Re: [PATCH] diff: fix out-of-bounds reads and NULL deref in diffstat UTF-8 truncation
Message-ID: <CABPp-BHt-O=CCnGHjoXBOHCe5CbD7beyrd_gX51g9Xg7cn_eFg@mail.gmail.com>
In-Reply-To: <xmqqv7dpwfy5.fsf@gitster.g>

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.