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

["René Scharfe" <l.s.r@web.de>]

Am 19.02.2016 um 12:22 schrieb Jeff King:
> REALLOC_ARRAY inherently involves a multiplication which can
> overflow size_t, resulting in a much smaller buffer than we
> think we've allocated. We can easily harden it by using
> st_mult() to check for overflow.  Likewise, we can add
> ALLOC_ARRAY to do the same thing for xmalloc calls.

Good idea!

> xcalloc() should already be fine, because it takes the two
> factors separately, assuming the system calloc actually
> checks for overflow. However, before we even hit the system
> calloc(), we do our memory_limit_check, which involves a
> multiplication. Let's check for overflow ourselves so that
> this limit cannot be bypassed.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   git-compat-util.h | 3 ++-
>   wrapper.c         | 3 +++
>   2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 0c65033..55c073d 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -779,7 +779,8 @@ extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);
>   extern char *xgetcwd(void);
>   extern FILE *fopen_for_writing(const char *path);
>
> -#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?

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. :)

René