On Tue, Sep 22, 2015 at 12:30 PM, Julian Foad
<julianf...@btopenworld.com> wrote:
> Bert Huijben wrote:
>> Stefan Sperling wrote:
>>> > + unsigned info = 0;
> [...]
>>> > + for (i = 0; i < 5; i++)
>>> > + {
>>> > + int value;
>>> > +
>>> > + SVN_ERR(base85_value(&value, base85_data[i]));
>>> > + info *= 85;
>>>
>>> What happens if 'info' overflows here?
>>
>> Probably something similar to the git implementation... different final
>> output; most likely 100% identical on all systems. Something that would
>> happen on every unexpected bad value in a diff file.
>>
>>> > + info += value;
>>> > + }
>
> Just to add a few more words of explanation, for anybody else
> wondering about this...
>
> 85^5 is 4437053125, a little greater than 256^4 = 2^32 = 4294967296,
> so 5 characters of base-85 encoded text is just a little more than
> enough to encode 4 bytes of raw data.
>
> 'info' needs to be at least 32 bits wide. If it is exactly 32 bits
> then it could overflow here if the current group of five base-85
> characters is an invalid combination -- or, we could just as well say,
> if it is a non-canonical encoding of a smallish 32-bit value. In the
> latter interpretation, it will produce an output that we could say is
> perfectly valid, a correct decoding of the (non-canonical) input.
>
> Do we care about raising an error in that case? I don't know. It
> doesn't seem particularly important to me, one way or the other.
Without completely understanding how the binary diff / patch is now
implemented, I'd say: better to error out in this case. If it helps
detecting corruption of the binary diff (say it was randomly changed
by some wire transmission) ...
--
Johan
