From 3131977de1476185209d4d56a0494f5b30ee5557 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Sep 2015 17:05:57 -0400 Subject: progress: store throughput display in a strbuf Coverity noticed that we strncpy() into a fixed-size buffer without making sure that it actually ended up NUL-terminated. This is unlikely to be a bug in practice, since throughput strings rarely hit 32 characters, but it would be nice to clean it up. The most obvious way to do so is to add a NUL-terminator. But instead, this patch switches the fixed-size buffer out for a strbuf. At first glance this seems much less efficient, until we realize that filling in the fixed-size buffer is done by writing into a strbuf and copying the result! By writing straight to the buffer, we actually end up more efficient: 1. We avoid an extra copy of the bytes. 2. Rather than malloc/free each time progress is shown, we can strbuf_reset and use the same buffer each time. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- progress.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) (limited to 'progress.c') diff --git a/progress.c b/progress.c index 2e31bec60f..a3efcfd34c 100644 --- a/progress.c +++ b/progress.c @@ -25,7 +25,7 @@ struct throughput { unsigned int last_bytes[TP_IDX_MAX]; unsigned int last_misecs[TP_IDX_MAX]; unsigned int idx; - char display[32]; + struct strbuf display; }; struct progress { @@ -98,7 +98,7 @@ static int display(struct progress *progress, unsigned n, const char *done) } progress->last_value = n; - tp = (progress->throughput) ? progress->throughput->display : ""; + tp = (progress->throughput) ? progress->throughput->display.buf : ""; eol = done ? done : " \r"; if (progress->total) { unsigned percent = n * 100 / progress->total; @@ -129,6 +129,7 @@ static int display(struct progress *progress, unsigned n, const char *done) static void throughput_string(struct strbuf *buf, off_t total, unsigned int rate) { + strbuf_reset(buf); strbuf_addstr(buf, ", "); strbuf_humanise_bytes(buf, total); strbuf_addstr(buf, " | "); @@ -141,7 +142,6 @@ void display_throughput(struct progress *progress, off_t total) struct throughput *tp; uint64_t now_ns; unsigned int misecs, count, rate; - struct strbuf buf = STRBUF_INIT; if (!progress) return; @@ -154,6 +154,7 @@ void display_throughput(struct progress *progress, off_t total) if (tp) { tp->prev_total = tp->curr_total = total; tp->prev_ns = now_ns; + strbuf_init(&tp->display, 0); } return; } @@ -193,9 +194,7 @@ void display_throughput(struct progress *progress, off_t total) tp->last_misecs[tp->idx] = misecs; tp->idx = (tp->idx + 1) % TP_IDX_MAX; - throughput_string(&buf, total, rate); - strncpy(tp->display, buf.buf, sizeof(tp->display)); - strbuf_release(&buf); + throughput_string(&tp->display, total, rate); if (progress->last_value != -1 && progress_update) display(progress, progress->last_value, NULL); } @@ -250,12 +249,9 @@ void stop_progress_msg(struct progress **p_progress, const char *msg) bufp = (len < sizeof(buf)) ? buf : xmalloc(len + 1); if (tp) { - struct strbuf strbuf = STRBUF_INIT; unsigned int rate = !tp->avg_misecs ? 0 : tp->avg_bytes / tp->avg_misecs; - throughput_string(&strbuf, tp->curr_total, rate); - strncpy(tp->display, strbuf.buf, sizeof(tp->display)); - strbuf_release(&strbuf); + throughput_string(&tp->display, tp->curr_total, rate); } progress_update = 1; sprintf(bufp, ", %s.\n", msg); @@ -264,6 +260,8 @@ void stop_progress_msg(struct progress **p_progress, const char *msg) free(bufp); } clear_progress_signal(); + if (progress->throughput) + strbuf_release(&progress->throughput->display); free(progress->throughput); free(progress); } -- cgit v1.3 From f5691aa640ae2c1c5f85037e093de7f310c0eac7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Sep 2015 17:06:46 -0400 Subject: stop_progress_msg: convert sprintf to xsnprintf The usual arguments for using xsnprintf over sprintf apply, but this case is a little tricky. We print to a fixed-size buffer if we have room, and otherwise to an allocated buffer. So there should be no overflow here, but it is still good to communicate our intention, as well as to check our earlier math for how much space the string will need. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- progress.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'progress.c') diff --git a/progress.c b/progress.c index a3efcfd34c..353bd37416 100644 --- a/progress.c +++ b/progress.c @@ -254,7 +254,7 @@ void stop_progress_msg(struct progress **p_progress, const char *msg) throughput_string(&tp->display, tp->curr_total, rate); } progress_update = 1; - sprintf(bufp, ", %s.\n", msg); + xsnprintf(bufp, len + 1, ", %s.\n", msg); display(progress, progress->last_value, bufp); if (buf != bufp) free(bufp); -- cgit v1.3