Re: [PATCH] linear-assignment: fix potential out of bounds memory access (was: Re: Git 2.19 Segmentation fault 11 on macOS)
- From
Thomas Gummerer <t.gummerer@gmail.com>
- Date
- Sep 13, 2018, 22:13 UTC
- Message-ID
- <20180913221318.GE1719@hank.intra.tgummerer.com>
- In-Reply-To
- <nycvar.QRO.7.76.6.1809122136020.73@tvgsbejvaqbjf.bet>
On 09/12, Johannes Schindelin wrote:
> Hi Thomas, > > [quickly, as I will go back to a proper vacation after this]
Sorry about interrupting your vacation, enjoy wherever you are! :)
Show 15 quoted lines
> On Wed, 12 Sep 2018, Thomas Gummerer wrote:
>
> > diff --git a/linear-assignment.c b/linear-assignment.c
> > index 9b3e56e283..7700b80eeb 100644
> > --- a/linear-assignment.c
> > +++ b/linear-assignment.c
> > @@ -51,8 +51,8 @@ void compute_assignment(int column_count, int row_count, int *cost,
> > else if (j1 < -1)
> > row2column[i] = -2 - j1;
> > else {
> > - int min = COST(!j1, i) - v[!j1];
> > - for (j = 1; j < column_count; j++)
> > + int min = INT_MAX;
>
> I am worried about this, as I tried very hard to avoid integer overruns.Ah fair enough, now I think I understand where the calculation of the initial value of min comes from, thanks!
> Wouldn't it be possible to replace the `else {` by an appropriate `else if
> (...) { ... } else {`? E.g. `else if (column_count < 2)` or some such?Yes, I think that would be possible. However if we're already special casing "column_count < 2", I think we might as well just exit early before running through the whole algorithm in that case. If there's only one column, there are no commits that can be assigned to eachother, as there is only the one.
We could also just not run call 'compute_assignment' in the first place if column_count == 1, however I'd rather make the function safer to call, just in case we find it useful for something else in the future.
Will send an updated patch in a bit.
Show 25 quoted lines
> Ciao,
> Dscho
>
> > + for (j = 0; j < column_count; j++)
> > if (j != j1 && min > COST(j, i) - v[j])
> > min = COST(j, i) - v[j];
> > v[j1] -= min;
> > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> > index 2237c7f4af..fb4c13a84a 100755
> > --- a/t/t3206-range-diff.sh
> > +++ b/t/t3206-range-diff.sh
> > @@ -142,4 +142,9 @@ test_expect_success 'changed message' '
> > test_cmp expected actual
> > '
> >
> > +test_expect_success 'no commits on one side' '
> > + git commit --amend -m "new message" &&
> > + git range-diff master HEAD@{1} HEAD
> > +'
> > +
> > test_done
> > --
> > 2.19.0.397.gdd90340f6a
> >
> >