Re: [PATCH 04/21] harden REALLOC_ARRAY and xcalloc against size_t overflow

[Jeff King <peff@peff.net>]

On Sat, Feb 20, 2016 at 10:32:00PM +0100, René Scharfe wrote:

> >-#define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
> >+#define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult((alloc), sizeof(*(x))))
> >+#define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult((alloc), sizeof(*(x))))
> 
> st_mult(x, y) calls unsigned_mult_overflows(x, y), which divides by x. This
> division can be done at compile time if x is a constant.  This can be
> guaranteed for all users of the two macros above by reversing the arguments
> of st_mult(), so that sizeof comes first.  Probably not a big win, but why
> not do it if it's that easy?

I doubt it's even measurable, but as you say, it's easy enough to do, so
why not.

If we really care about optimizing, I suspect that something like:

diff --git a/git-compat-util.h b/git-compat-util.h
index 5cbdd2e..1d53328 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -662,10 +662,12 @@ static inline size_t st_add(size_t a, size_t b)
 
 static inline size_t st_mult(size_t a, size_t b)
 {
-	if (unsigned_mult_overflows(a, b))
+	size_t ret;
+
+	if (__builtin_mul_overflow(a, b, &ret))
 		die("size_t overflow: %"PRIuMAX" * %"PRIuMAX,
 		    (uintmax_t)a, (uintmax_t)b);
-	return a * b;
+	return ret;
 }
 
 static inline size_t st_sub(size_t a, size_t b)


would do a lot more. But it needs #ifdefs for compilers besides gcc and
clang.

> Or perhaps a macro like this could help here and in other places which use
> st_mult with sizeof:
> 
>   #define SIZEOF_MULT(x, n) st_mult(sizeof(x), (n))
> 
> (I'd call it ARRAY_SIZE, but that name is already taken. :)

I don't think we need that; we're really only checking allocations,
which means ALLOC_ARRAY() and friends cover all the uses of sizeof (we
might still use st_mult() for _another_ part of the computation, but it
won't usually be a sizeof then).

We also may do follow-up multiplications, like:

  ALLOC_ARRAY(foo, nr);
  memset(foo, 0, nr * sizeof(*foo));

but I didn't bother doing overflow checks for those. We know that they
should be fine if the original allocation was successful (though of
course, just using xcalloc here would be better still).

-Peff