Re: [PATCH 4/4] Fix delta offset overflow
- Date: Fri, 11 Aug 2017 11:50:13 -0700
- From: Junio C Hamano <gitster@xxxxxxxxx>
- Subject: Re: [PATCH 4/4] Fix delta offset overflow
Martin Koegler <martin.koegler@xxxxxxxxx> writes:
> On Thu, Aug 10, 2017 at 01:49:24PM -0700, Junio C Hamano wrote:
>> 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?
> moff is otherwise only decremented or assigned with an offset generated by
> create_delta_index. These offsets are limited by 4GB.
> Any larger offets would be a programming bug - so qualify for just a "assert".
OK, in that case, I agree that a check before encoding moff into
(upto) 4 output bytes is unnecessary. Sorry, I didn't read the
function that populates index->hash before responding, and I admit
that I haven't read it for a while.
>> 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...
> The current code produces incorrect deltas - its not just a checksum issue.
Again, I mis-read what role msize was playing in the original (or in
your update). I'd need to re-read that part of the code to make
sure I get how your change will fix the issue.
> By the way:
> Somebody interested in JGIT should also look at these two bugs:
> copy would also encode beyond 4GB - producing truncated delta offset.
> apply uses int for decoding length values.
I'll cc: an obvious suspect; thanks for the note.