Corrected & refactored handling of window bar content...

Old snprintf calls could have overflowed the buffers.

3 files changed, 67 insertions, 41 deletions

diff --git a/main.c b/main.c
index 31effbd..46d80aa 100644
--- a/main.c
+++ b/main.c
@@ -21,6 +21,7 @@
 
 #include <stdlib.h>
 #include <stdio.h>
+#include <stdarg.h>
 #include <string.h>
 #include <fcntl.h>
 #include <unistd.h>
@@ -256,7 +257,7 @@ void open_info(void)
 		kill(pid, SIGTERM);
 		info.fd = -1;
 	}
-	win.bar.l[0] = '\0';
+	win.bar.l.buf[0] = '\0';
 
 	if (pipe(pfd) < 0)
 		return;
@@ -290,20 +291,20 @@ void read_info(void)
 		for (i = 0; i < n; i++) {
 			if (buf[i] == '\n') {
 				if (info.lastsep == 0) {
-					win.bar.l[info.i++] = ' ';
+					win.bar.l.buf[info.i++] = ' ';
 					info.lastsep = 1;
 				}
 			} else {
-				win.bar.l[info.i++] = buf[i];
+				win.bar.l.buf[info.i++] = buf[i];
 				info.lastsep = 0;
 			}
-			if (info.i + 1 == sizeof(win.bar.l))
+			if (info.i + 1 == win.bar.l.size)
 				goto end;
 		}
 	}
 end:
 	info.i -= info.lastsep;
-	win.bar.l[info.i] = '\0';
+	win.bar.l.buf[info.i] = '\0';
 	win_draw(&win);
 	close(info.fd);
 	info.fd = -1;
@@ -343,15 +344,24 @@ void load_image(int new)
 		reset_timeout(animate);
 }
 
+void bar_put(win_bar_t *bar, const char *fmt, ...)
+{
+	size_t len = bar->size - (bar->p - bar->buf), n;
+	va_list ap;
+
+	va_start(ap, fmt);
+	n = vsnprintf(bar->p, len, fmt, ap);
+	bar->p += MIN(len, n);
+	va_end(ap);
+}
+
 void update_info(void)
 {
-	unsigned int i, fn, fw, n;
-	unsigned int llen = sizeof(win.bar.l), rlen = sizeof(win.bar.r);
-	char *lt = win.bar.l, *rt = win.bar.r, title[TITLE_LEN];
+	unsigned int i, fn, fw;
+	char title[TITLE_LEN];
 	const char * mark;
 	bool ow_info;
-
-	for (fw = 0, i = filecnt; i > 0; fw++, i /= 10);
+	win_bar_t *l = &win.bar.l, *r = &win.bar.r;
 
 	/* update window title */
 	if (mode == MODE_THUMB) {
@@ -364,39 +374,41 @@ void update_info(void)
 	/* update bar contents */
 	if (win.bar.h == 0)
 		return;
+	for (fw = 0, i = filecnt; i > 0; fw++, i /= 10);
 	mark = files[fileidx].marked ? "* " : "";
+	l->p = l->buf;
+	r->p = r->buf;
 	if (mode == MODE_THUMB) {
 		if (tns.loadnext < tns.end) {
-			snprintf(lt, llen, "Loading... %0*d", fw, tns.loadnext);
+			bar_put(l, "Loading... %0*d", fw, MAX(tns.loadnext, 1));
 			ow_info = false;
 		} else {
 			ow_info = true;
 		}
-		n = snprintf(rt, rlen, "%s%0*d/%d", mark, fw, fileidx + 1, filecnt);
+		bar_put(r, "%s%0*d/%d", mark, fw, fileidx + 1, filecnt);
 	} else {
-		n = snprintf(rt, rlen, "%s", mark);
+		bar_put(r, "%s", mark);
 		if (img.ss.on)
-			n += snprintf(rt + n, rlen - n, "%ds | ", img.ss.delay);
+			bar_put(r, "%ds | ", img.ss.delay);
 		if (img.gamma != 0)
-			n += snprintf(rt + n, rlen - n, "G%+d | ", img.gamma);
-		n += snprintf(rt + n, rlen - n, "%3d%% | ", (int) (img.zoom * 100.0));
+			bar_put(r, "G%+d | ", img.gamma);
+		bar_put(r, "%3d%% | ", (int) (img.zoom * 100.0));
 		if (img.multi.cnt > 0) {
 			for (fn = 0, i = img.multi.cnt; i > 0; fn++, i /= 10);
-			n += snprintf(rt + n, rlen - n, "%0*d/%d | ",
-			              fn, img.multi.sel + 1, img.multi.cnt);
+			bar_put(r, "%0*d/%d | ", fn, img.multi.sel + 1, img.multi.cnt);
 		}
-		n += snprintf(rt + n, rlen - n, "%0*d/%d", fw, fileidx + 1, filecnt);
+		bar_put(r, "%0*d/%d", fw, fileidx + 1, filecnt);
 		ow_info = info.cmd == NULL;
 	}
 	if (ow_info) {
 		fn = strlen(files[fileidx].name);
-		if (fn < llen &&
+		if (fn < l->size &&
 		    win_textwidth(files[fileidx].name, fn, true) +
-		    win_textwidth(rt, n, true) < win.w)
+		    win_textwidth(r->buf, r->p - r->buf, true) < win.w)
 		{
-			strncpy(lt, files[fileidx].name, llen);
+			strncpy(l->buf, files[fileidx].name, l->size);
 		} else {
-			strncpy(lt, files[fileidx].base, llen);
+			strncpy(l->buf, files[fileidx].base, l->size);
 		}
 	}
 }
@@ -469,7 +481,7 @@ void run_key_handler(const char *key, unsigned int mask)
 	int i, j, retval, status;
 	int fcnt = mode == MODE_THUMB && markcnt > 0 ? markcnt : 1;
 	bool changed = false;
-	char **args, kstr[32], oldbar[sizeof(win.bar.l)];
+	char **args, kstr[32], oldbar[BAR_L_LEN];
 	struct stat *oldst, newst;
 	struct { int fn; struct stat st; } *finfo;
 
@@ -504,8 +516,8 @@ void run_key_handler(const char *key, unsigned int mask)
 	         mask & Mod1Mask    ? "M-" : "",
 	         mask & ShiftMask   ? "S-" : "", key);
 
-	memcpy(oldbar, win.bar.l, sizeof(win.bar.l));
-	strncpy(win.bar.l, "Running key handler...", sizeof(win.bar.l));
+	memcpy(oldbar, win.bar.l.buf, sizeof(oldbar));
+	strncpy(win.bar.l.buf, "Running key handler...", win.bar.l.size);
 	win_draw(&win);
 	win_set_cursor(&win, CURSOR_WATCH);
 
@@ -540,7 +552,7 @@ end:
 			img_close(&img, true);
 			load_image(fileidx);
 		} else if (info.cmd != NULL) {
-			memcpy(win.bar.l, oldbar, sizeof(win.bar.l));
+			memcpy(win.bar.l.buf, oldbar, win.bar.l.size);
 		}
 	}
 	reset_cursor();
diff --git a/window.c b/window.c
index 581ca76..ff7f116 100644
--- a/window.c
+++ b/window.c
@@ -168,7 +168,12 @@ void win_init(win_t *win)
 	win->selcol    = win_alloc_color(win, SEL_COLOR);
 	win->bar.bgcol = win_alloc_color(win, BAR_BG_COLOR);
 	win->bar.fgcol = win_alloc_color(win, BAR_FG_COLOR);
-	win->bar.h     = options->hide_bar ? 0 : barheight;
+
+	win->bar.l.size = BAR_L_LEN;
+	win->bar.r.size = BAR_R_LEN;
+	win->bar.l.buf = s_malloc(win->bar.l.size);
+	win->bar.r.buf = s_malloc(win->bar.r.size);
+	win->bar.h = options->hide_bar ? 0 : barheight;
 
 	INIT_ATOM_(WM_DELETE_WINDOW);
 	INIT_ATOM_(_NET_WM_NAME);
@@ -416,9 +421,12 @@ void win_draw_bar(win_t *win)
 	char rest[3];
 	const char *dots = "...";
 	win_env_t *e;
+	win_bar_t *l, *r;
 
 	if (win == NULL || win->xwin == None)
 		return;
+	if ((l = &win->bar.l)->buf == NULL || (r = &win->bar.r)->buf == NULL)
+		return;
 
 	e = &win->env;
 	y = win->h + font.ascent + V_TEXT_PAD;
@@ -430,35 +438,35 @@ void win_draw_bar(win_t *win)
 	XSetForeground(e->dpy, gc, win->bar.fgcol);
 	XSetBackground(e->dpy, gc, win->bar.bgcol);
 
-	if ((len = strlen(win->bar.r)) > 0) {
-		if ((tw = win_textwidth(win->bar.r, len, true)) > w)
+	if ((len = strlen(r->buf)) > 0) {
+		if ((tw = win_textwidth(r->buf, len, true)) > w)
 			return;
 		x = win->w - tw + H_TEXT_PAD;
 		w -= tw;
 		if (font.set)
-			XmbDrawString(e->dpy, win->buf.pm, font.set, gc, x, y, win->bar.r, len);
+			XmbDrawString(e->dpy, win->buf.pm, font.set, gc, x, y, r->buf, len);
 		else
-			XDrawString(e->dpy, win->buf.pm, gc, x, y, win->bar.r, len);
+			XDrawString(e->dpy, win->buf.pm, gc, x, y, r->buf, len);
 	}
-	if ((len = strlen(win->bar.l)) > 0) {
+	if ((len = strlen(l->buf)) > 0) {
 		olen = len;
-		while (len > 0 && (tw = win_textwidth(win->bar.l, len, true)) > w)
+		while (len > 0 && (tw = win_textwidth(l->buf, len, true)) > w)
 			len--;
 		if (len > 0) {
 			if (len != olen) {
 				w = strlen(dots);
 				if (len <= w)
 					return;
-				memcpy(rest, win->bar.l + len - w, w);
-				memcpy(win->bar.l + len - w, dots, w);
+				memcpy(rest, l->buf + len - w, w);
+				memcpy(l->buf + len - w, dots, w);
 			}
 			x = H_TEXT_PAD;
 			if (font.set)
-				XmbDrawString(e->dpy, win->buf.pm, font.set, gc, x, y, win->bar.l, len);
+				XmbDrawString(e->dpy, win->buf.pm, font.set, gc, x, y, l->buf, len);
 			else
-				XDrawString(e->dpy, win->buf.pm, gc, x, y, win->bar.l, len);
+				XDrawString(e->dpy, win->buf.pm, gc, x, y, l->buf, len);
 			if (len != olen)
-			  memcpy(win->bar.l + len - w, rest, w);
+			  memcpy(l->buf + len - w, rest, w);
 		}
 	}
 }
diff --git a/window.h b/window.h
index 1178007..8761120 100644
--- a/window.h
+++ b/window.h
@@ -50,6 +50,12 @@ typedef struct {
 } win_env_t;
 
 typedef struct {
+	size_t size;
+	char *p;
+	char *buf;
+} win_bar_t;
+
+typedef struct {
 	Window xwin;
 	win_env_t env;
 
@@ -73,8 +79,8 @@ typedef struct {
 
 	struct {
 		unsigned int h;
-		char l[BAR_L_LEN];
-		char r[BAR_R_LEN];
+		win_bar_t l;
+		win_bar_t r;
 		unsigned long bgcol;
 		unsigned long fgcol;
 	} bar;