Re: [PATCH 2/5] diff --color-moved-ws: fix out of bounds string access

From: Stefan Beller <sbeller@google.com>
To: Phillip Wood <phillip.wood@dunelm.org.uk>
Cc: git <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 2/5] diff --color-moved-ws: fix out of bounds string access
Date: Tue, 2 Oct 2018 11:58:45 -0700	[thread overview]
Message-ID: <CAGZ79kYu9enUT=yL8wfM9quYYz1fs5OU4Wcvci9ZVPPdXkY6gA@mail.gmail.com> (raw)
In-Reply-To: <20181002175514.31495-2-phillip.wood@talktalk.net>

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; ...)