Re: [PATCH v2 1/2] bugfix: fix broken time_buf paddings for git-blame

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Apr 20, 2014 at 12:13 PM, Jiang Xin <worldhello.net@gmail.com> wrote:
> When `git blame` shows date field in a fixed width (as long as

s/fixed width/fixed-width/

s/long/wide/ would read a bit better.

> blame_date_width characters), if time_str shorter than that, add spaces

s/shorter/is shorter/
s/add/it adds/

> for padding.  But there are two bugs in the following codes:
>
>         memcpy(time_buf, time_str, time_len);
>         memset(time_buf + time_len, ' ', blame_date_width - time_len);
>
> 1. The type of blame_date_width is size_t (unsigned int).  If time_len

s/(unsigned int)/, which is unsigned/

>    is greater than blame_ate_width, "blame_date_width - time_len" will

s/_ate/_date/

>    never be a negative number, but a really big positive number, and
>    will cause memory overwrite.
>
>    This bug can be triggered if either l10n message for function
>    show_date_relative() in date.c is longer then 30 charcters, then

s/then 30/than 30/
s/charcters/characters/

>    `git blame --date relative` may fail.
>
> 2. When show blame information with relative time, the UTF-8 characters
>    in time_str will break the alignment of columns after the date field.
>    This is because the time_buf padding with spaces should have a
>    constant display width, not a fixed strlen size.  So we should call
>    utf8_strwidth() instead of strlen() for calibration.
>
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> ---
>  builtin/blame.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 88cb799..0a0a858 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1556,22 +1556,31 @@ static void assign_blame(struct scoreboard *sb, int opt)
>  static const char *format_time(unsigned long time, const char *tz_str,
>                                int show_raw_time)
>  {
> -       static char time_buf[128];
> +       static struct strbuf time_buf = STRBUF_INIT;
>
> +       strbuf_reset(&time_buf);
>         if (show_raw_time) {
> -               snprintf(time_buf, sizeof(time_buf), "%lu %s", time, tz_str);
> +               strbuf_addf(&time_buf, "%lu %s", time, tz_str);
>         }
>         else {
>                 const char *time_str;
> +               size_t time_width;
>                 int time_len;
>                 int tz;
>                 tz = atoi(tz_str);
>                 time_str = show_date(time, tz, blame_date_mode);
>                 time_len = strlen(time_str);
> -               memcpy(time_buf, time_str, time_len);
> -               memset(time_buf + time_len, ' ', blame_date_width - time_len);
> +               strbuf_add(&time_buf, time_str, time_len);
> +               /*
> +                * Add space paddings to time_buf to display a fixed width
> +                * string, and use time_width for display width calibration.
> +                */
> +               for (time_width = utf8_strwidth(time_str);
> +                    time_width < blame_date_width;
> +                    time_width++)
> +                       strbuf_addch(&time_buf, ' ');
>         }
> -       return time_buf;
> +       return time_buf.buf;
>  }
>
>  #define OUTPUT_ANNOTATE_COMPAT 001
> --
> 2.0.0.rc0.3.g444188f.dirty