Re: [PATCH] Fix delta integer overflows

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

Martin Koegler <martin.koegler@chello.at> writes:

> From: Martin Koegler <martin.koegler@chello.at>
>
> The current delta code produces incorrect pack objects for files > 4GB.
>
> Signed-off-by: Martin Koegler <martin.koegler@chello.at>
> ---
>  diff-delta.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> Just pass any file > 4 GB to the delta-compression [by increasing the delta limits].
> As file size, a truncated 32bit value will be encoded, leading to broken pack files.

The patch obviously makes the code better and self consistent in
that "struct delta_index" has src_size as ulong, and this function
takes trg_size as ulong, and it was plain wrong for the code to
assume that "i", which is uint, can receive it safely.

In the longer term we might want to move to size_t or even
uintmax_t, as the ulong on a platform may not be long enough in
order to express the largest file size the platform can have, but
this patch (1) is good even without such a change, and (2) gives a
good foundation to build on if we want such a change on top.

Thanks.  Will queue.

>
> diff --git a/diff-delta.c b/diff-delta.c
> index 3797ce6..13e5a01 100644
> --- a/diff-delta.c
> +++ b/diff-delta.c
> @@ -319,7 +319,8 @@ create_delta(const struct delta_index *index,
>  	     const void *trg_buf, unsigned long trg_size,
>  	     unsigned long *delta_size, unsigned long max_size)
>  {
> -	unsigned int i, outpos, outsize, moff, msize, val;
> +	unsigned int i, val;
> +	unsigned long l, outpos, outsize, moff, msize;
>  	int inscnt;
>  	const unsigned char *ref_data, *ref_top, *data, *top;
>  	unsigned char *out;
> @@ -336,20 +337,20 @@ create_delta(const struct delta_index *index,
>  		return NULL;
>  
>  	/* store reference buffer size */
> -	i = index->src_size;
> -	while (i >= 0x80) {
> -		out[outpos++] = i | 0x80;
> -		i >>= 7;
> +	l = index->src_size;
> +	while (l >= 0x80) {
> +		out[outpos++] = l | 0x80;
> +		l >>= 7;
>  	}
> -	out[outpos++] = i;
> +	out[outpos++] = l;
>  
>  	/* store target buffer size */
> -	i = trg_size;
> -	while (i >= 0x80) {
> -		out[outpos++] = i | 0x80;
> -		i >>= 7;
> +	l = trg_size;
> +	while (l >= 0x80) {
> +		out[outpos++] = l | 0x80;
> +		l >>= 7;
>  	}
> -	out[outpos++] = i;
> +	out[outpos++] = l;
>  
>  	ref_data = index->src_buf;
>  	ref_top = ref_data + index->src_size;