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

Message ID 20181002175514.31495-2-phillip.wood@talktalk.net
State New
Headers show
Series
  • [1/5] diff --color-moved-ws: fix double free crash
Related show

Commit Message

Phillip Wood Oct. 2, 2018, 5:55 p.m. UTC
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(-)

Comments

Stefan Beller Oct. 2, 2018, 6:58 p.m. UTC | #1
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; ...)
Phillip Wood Oct. 3, 2018, 9:40 a.m. UTC | #2
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

Patch
diff mbox series

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;