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

From: Phillip Wood <phillip.wood@talktalk.net>
Date: Oct 3, 2018, 09:40 UTC
Message-ID: <ea197dba-bb51-0dde-bd2b-84709ce81a51@talktalk.net>
In-Reply-To: <CAGZ79kYu9enUT=yL8wfM9quYYz1fs5OU4Wcvci9ZVPPdXkY6gA@mail.gmail.com>

On 02/10/2018 19:58, Stefan Beller wrote:

Show 27 quoted lines

> 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

Show 9 quoted lines

> 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