Re: [PATCH v3] diff-delta: use safe integer arithmetic

[Junio C Hamano <gitster@pobox.com>]

"Srujith A Poojary via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Srujith A Poojary <poojarysrujith@gmail.com>
>
> Replace manual multiplication and addition in create_delta_index()
> with safe integer helpers (st_mult, st_add, st_add3) to prevent potential
> overflows when computing memory allocation sizes.
>
> On 32-bit platforms, struct alignment padding could cause the total
> allocation size calculation to exceed 2^32, making safe integer arithmetic
> necessary.

I wonder if "X could cause Y" above skips a bit too many steps.  I
was expecting more along the lines of ...

    We will need to allocate N instances of A and M instances of B, N
    and M are capped to X and Y respectively by this code

        ... code snippet ...

    and size of an instance of A is 8 and size of B could be either W or
    Z.  When size of B is W, then the total allocation would be ...

And that analysis applies only on the first allocation.  I didn't
even look at the packed side of the allocation size, expecting that
you would know how to analyze after seeing my example on the other
allocation.

> Note that hsize + 1 inside st_add3() is safe from overflow
> because hsize is capped at 2^26 early in the function.

It is misleading to say hsize+1 is inside st_add3(), though.


> diff --git a/diff-delta.c b/diff-delta.c
> index 7cbedeb507..efebce3aca 100644
> --- a/diff-delta.c
> +++ b/diff-delta.c
> @@ -162,8 +162,8 @@ struct delta_index * create_delta_index(const void *buf, size_t bufsize)
>  	hmask = hsize - 1;
>  
>  	/* allocate lookup index */
> -	memsize = sizeof(*hash) * hsize +
> -		  sizeof(*entry) * entries;
> +	memsize = st_add(st_mult(sizeof(*hash), hsize),
> +			 st_mult(sizeof(*entry), entries));
>  	mem = malloc(memsize);
>  	if (!mem)
>  		return NULL;
> @@ -258,9 +258,9 @@ struct delta_index * create_delta_index(const void *buf, size_t bufsize)
>  	 * Now create the packed index in array form
>  	 * rather than linked lists.
>  	 */
> -	memsize = sizeof(*index)
> -		+ sizeof(*packed_hash) * (hsize+1)
> -		+ sizeof(*packed_entry) * entries;
> +	memsize = st_add3(sizeof(*index),
> +			  st_mult(sizeof(*packed_hash), hsize + 1),
> +			  st_mult(sizeof(*packed_entry), entries));
>  	mem = malloc(memsize);
>  	if (!mem) {
>  		free(hash);
>
> base-commit: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926