From: Mark Jamsek <mark@jamsek.com>
Subject: Re: [rfc] tog horizontal scroll (diff & blame view)
To: Theo Buehler <tb@theobuehler.org>
Cc: gameoftrees@openbsd.org
Date: Thu, 16 Jun 2022 20:27:55 +1000

On 22-06-16 10:25am, Theo Buehler wrote:
> > @@ -1223,21 +1224,62 @@ done:
> >  	return err;
> >  }
> > 
> > +static const struct got_error *
> > +expand_tab(char **ptr, const char *src)
> > +{
> > +	char	*dst;
> > +	size_t	 len, n, idx = 0, sz = 0;
> > +
> > +	*ptr = NULL;
> > +	n = len = strlen(src);
> > +	dst = malloc((n + 1) * sizeof(char));
> 
> This looks like a potential overflow due to multiplying sizes in malloc().
> Since sizeof(char) == 1 by definition that's not an issue. For the same
> reason, I would simplify this to
> 
> 	dst = malloc(n + 1);
> 
> > +	if (dst == NULL)
> > +		return got_error_from_errno("malloc");
> > +
> > +	while (idx < len && src[idx]) {
> > +		const char c = src[idx];
> > +
> > +		if (c == '\t') {
> > +			size_t nb = TABSIZE - sz % TABSIZE;
> > +			n += nb;
> > +			dst = reallocarray(dst, n, sizeof(char));
> > +			if (dst == NULL)
> > +				return got_error_from_errno("reallocarray");
> 
> This leaks dst on reallocarray() failure and should use an intermediate
> pointer. Also, I think it would be better to grow n only after the
> reallocation succeeded:
> 
> 			p = realloc(dst, n + nb);
> 			if (p == NULL) {
> 				free(dst);
> 				return got_error_from_errno("realloc");
> 			}
> 			dst = p;
> 			n += nb;
> 
> 
> > +			memcpy(dst + sz, "        ", nb);
> 
> I would use
> 			memset(dst + sz, ' ', nb);
> 
> > +			sz += nb;
> > +		} else
> > +			dst[sz++] = src[idx];
> > +		++idx;
> > +	}
> 

Thanks, Theo! I appreciate the tips. Revised patch is below:

diff a75b3e2dd76c9392eb52ae19ef339a13f32eb821 /Users/mark/Library/Mobile Documents/com~apple~CloudDocs/src/got-hscroll
blob - 8a28476cd4abc2585e93d816ed8368b82925038b
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -1232,7 +1232,7 @@ expand_tab(char **ptr, const char *src)

 	*ptr = NULL;
 	n = len = strlen(src);
-	dst = malloc((n + 1) * sizeof(char));
+	dst = malloc(n + 1);
 	if (dst == NULL)
 		return got_error_from_errno("malloc");

@@ -1241,11 +1241,15 @@ expand_tab(char **ptr, const char *src)

 		if (c == '\t') {
 			size_t nb = TABSIZE - sz % TABSIZE;
+			char *p = realloc(dst, n + nb);
+			if (p == NULL) {
+				free(dst);
+				return got_error_from_errno("realloc");
+
+			}
+			dst = p;
 			n += nb;
-			dst = reallocarray(dst, n, sizeof(char));
-			if (dst == NULL)
-				return got_error_from_errno("reallocarray");
-			memcpy(dst + sz, "        ", nb);
+			memset(dst + sz, ' ', nb);
 			sz += nb;
 		} else
 			dst[sz++] = src[idx];


-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68