Re: [PATCH] [Outreachy PATCH] 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
> overflows while preserving NULL returns on failure.

I do not quite understand what the last part of this explanation
wants to say, i.e., "while preserving NULL returns on failure".  It
makes it sound as if the function would return NULL if the integer
helpers detect integer overflow, but I do not think that is what
actually the updated code does at all.  st_add() and friends will
simply die() when overflow happens, so you will not even reach
malloc(), let alone return NULL when the malloc fails.

And if you reply to this review message, please do not top-post.

Thanks.

>
> Signed-off-by: Srujith A Poojary <poojarysrujith@gmail.com>
> ---
>     [Outreachy PATCH] diff-delta: use safe integer arithmetic
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2276%2Fpoojarysrujith%2Fc-refactor-target-name-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2276/poojarysrujith/c-refactor-target-name-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2276
>
>  diff-delta.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/diff-delta.c b/diff-delta.c
> index 7cbedeb507..45df070ab9 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