Web lists-archives.com

Re: [PATCH 4/4] Fix delta offset overflow




Martin Koegler <martin.koegler@xxxxxxxxx> writes:

> From: Martin Koegler <martin.koegler@xxxxxxxxx>
>
> Prevent generating delta offsets beyond 4G.
>
> Signed-off-by: Martin Koegler <martin.koegler@xxxxxxxxx>
> ---
>  diff-delta.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/diff-delta.c b/diff-delta.c
> index 3d5e1ef..633883e 100644
> --- a/diff-delta.c
> +++ b/diff-delta.c
> @@ -454,6 +454,9 @@ create_delta(const struct delta_index *index,
>  			moff += msize;
>  			msize = left;
>  
> +			if (moff > 0xffffffff)
> +				msize = 0;
> +

The lower 4-byte of moff (before incrementing it with msize) were
already encoded to the output stream before this hunk.  Shouldn't
we be checking if moff would fit in uint32_t _before_ that happens?

IOW, in a much earlier part of that "while (data < top)" loop, there
is a code that tries to find a match that gives us a large msize by
iterating over index->hash[], and it sets msize and moff as a potential
location that we may want to use.  If moff is already big there, then
we shouldn't consider it a match because we cannot encode its location
using 4-byte anyway, no?

Cutting it off at here by resetting msize to 0 might help the next
iteration (I didn't check, but is the effect of it is to corrupt the
"val" rolling checksum and make it unlikely that the hash
computation would not find a correct match?) but it somehow feels
like closing the barn door after the horse has already bolted...

>  			if (msize < 4096) {
>  				int j;
>  				val = 0;