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

2014-04-20 Thread Eric Sunshine
On Sun, Apr 20, 2014 at 12:13 PM, Jiang Xin  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 
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2014-04-20 Thread Jiang Xin
When `git blame` shows date field in a fixed width (as long as
blame_date_width characters), if time_str shorter than that, add spaces
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
   is greater than blame_ate_width, "blame_date_width - time_len" will
   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
   `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 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html