| Message ID | 20181002175514.31495-2-phillip.wood@talktalk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] diff --color-moved-ws: fix double free crash | expand |
On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood <phillip.wood@talktalk.net> wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > When adjusting the start of the string to take account of the change > in indentation the code was not checking that the string being > adjusted was in fact longer than the indentation change. This was > detected by asan. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > diff.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/diff.c b/diff.c > index 5a08d64497..0096bdc339 100644 > --- a/diff.c > +++ b/diff.c > @@ -841,7 +841,7 @@ static int cmp_in_block_with_wsd(const struct diff_options *o, > al -= wslen; > } > > - if (strcmp(a, c)) > + if (al < 0 || al != cl || memcmp(a, c, al)) If (al < 0) then al != cl, so I would think we could drop the first part, and only check with al != cl || memcmp In theory we should use xdiff_compare_lines here as the rest of the lines could have more white space issues, but we catch that earlier via a die("color-moved-ws: allow-indentation-change cannot be combined with other white space modes"), so memcmp is fine. Side note: There are comments above this code that seem to be also obsolete after patch 1 ("...carried forward in pmb->wsd; ...)
On 02/10/2018 19:58, Stefan Beller wrote: > On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood <phillip.wood@talktalk.net> wrote: >> >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> When adjusting the start of the string to take account of the change >> in indentation the code was not checking that the string being >> adjusted was in fact longer than the indentation change. This was >> detected by asan. >> >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> --- >> diff.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/diff.c b/diff.c >> index 5a08d64497..0096bdc339 100644 >> --- a/diff.c >> +++ b/diff.c >> @@ -841,7 +841,7 @@ static int cmp_in_block_with_wsd(const struct diff_options *o, >> al -= wslen; >> } >> >> - if (strcmp(a, c)) >> + if (al < 0 || al != cl || memcmp(a, c, al)) > > If (al < 0) then al != cl, so I would think we could drop the first > part, and only check with al != cl || memcmp Yes, I couldn't make up my mind weather to add the al < 0 or not but of course only one of the lengths can ever be negative, I'll remove it > In theory we should use xdiff_compare_lines here > as the rest of the lines could have more white space issues, > but we catch that earlier via a die("color-moved-ws: > allow-indentation-change cannot be combined with other > white space modes"), so memcmp is fine. > > Side note: There are comments above this code that seem > to be also obsolete after patch 1 ("...carried forward in > pmb->wsd; ...) Good point I'll go through the comments and update them Best Wishes Phillip
diff --git a/diff.c b/diff.c index 5a08d64497..0096bdc339 100644 --- a/diff.c +++ b/diff.c @@ -841,7 +841,7 @@ static int cmp_in_block_with_wsd(const struct diff_options *o, al -= wslen; } - if (strcmp(a, c)) + if (al < 0 || al != cl || memcmp(a, c, al)) return 1; return 0;