subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Johan Corveleyn <jcor...@gmail.com>
Subject Re: svn commit: r1704374 - in /subversion/trunk/subversion: include/svn_diff.h include/svn_error_codes.h libsvn_diff/binary_diff.c libsvn_diff/diff.h libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Date Wed, 23 Sep 2015 12:50:59 GMT
On Tue, Sep 22, 2015 at 12:30 PM, Julian Foad
<julianfoad@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

Mime
View raw message