[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Johan Corveleyn <jcorvel_at_gmail.com>
Date: Wed, 23 Sep 2015 14:50:59 +0200

On Tue, Sep 22, 2015 at 12:30 PM, Julian Foad
<julianfoad_at_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
Received on 2015-09-23 14:51:44 CEST

This is an archived mail posted to the Subversion Dev mailing list.