Re: [RFC PATCH 00/10] range-diff: fix segfault due to integer overflow

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Dec 21 2021, Philip Oakley wrote:

> Sorry for the late comment..
>
> On 10/12/2021 14:31, Johannes Schindelin wrote:
>> Hi Ævar,
>>
>> On Thu, 9 Dec 2021, Ævar Arnfjörð Bjarmason wrote:
>>
>>> The difference between "master" and "git-for-windows/main" is large
>>> enough that comparing the two will segfault on my system. This is
>>> because the range-diff code does some expensive calculations and will
>>> overflow the "int" type.
>> You are holding this thing wrong.
>>
>> The `main` branch of Git for Windows uses merging rebases, therefore you
>> need to use a commit range like
>> `git-for-windows/main^{/^Start.the.merging}..git-for-windows/main` and
>> compare it to `git-for-windows/main..master`.
>
> I'm not sure that a Git repo has an established way of indicating to how
> it's branching/merging/releasing workflow is set up, especially for
> projects with non-normative use cases, such as Git for Windows. We don't
> have a git document for covering  the different workflows in common use
> for easy reference and consistent terminology.
>
> The merging rebase flow, with 'fake' merge does solve a problem that
> git.git doesn't have but could easily be a common process for 'friendly
> forks' that follow an upstream with local patches. The choice of
> '{/^Start.the.merging}' is currently specific to the Git-for-Windows
> case making it harder to discover this useful maintainer method.

Yes, but let's not get lost in the weeds here. As I noted I just picked
GFW as a handy example of a large history & that command as a handy
example of something that segfaults on "master".

So the point really isn't to say that we should fix range-diff becase
it'll allow us to run this practically useful command on a git.git fork.

> I fully agree that the range-diff should probably have a patch limit at
> some sensible value.

Why would it? If I'm willing to spend the CPU to produce a range-diff of
an absurdly large range and I've got the memory why shouldn't we support
it?

We don't in cases like xdiff where it's not trivial to just raise the
limits, but here it seems relatively easy.

I think limits to save users from spending CPU time they didn't expect
are reasonable, but then we can handle them like the diff/merge rename
detection limits, i.e. print a warning/advice, and allow the user to
opt-out.

That also doesn't really apply here since "diff/merge" will/might still
do something useful in those scenarios, whereas range-diff would just
have truncated output.

> The 'confusion' between the types size_t, long and int, does ripple
> through a lot of portable code, as shown in the series. Not an easy problem.

Yes, although here we're not just casting and overflowing types, but
overflowing on multiplication and addition, whereas usually we'd just
overflow on "nr" being too big for "int" or similar.