diff mbox series

[3/5] diff --color-moved-ws: fix a memory leak

Message ID 20181002175514.31495-3-phillip.wood@talktalk.net (mailing list archive)
State New, archived
Headers show
Series [1/5] diff --color-moved-ws: fix double free crash | expand

Commit Message

Phillip Wood Oct. 2, 2018, 5:55 p.m. UTC 
From: Phillip Wood <phillip.wood@dunelm.org.uk>

Don't duplicate the indentation string if we're not going to use it.
This was found with asan.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 diff.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Stefan Beller Oct. 2, 2018, 7:08 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>
>
> Don't duplicate the indentation string if we're not going to use it.
> This was found with asan.

Makes sense,

Thanks,
Stefan

With compute_ws_delta growing bigger here (and having only one caller),
I wonder if we want to pass 'match' instead of match->es (and pmb_nr),
such that the function can also take care of
    pmb[pmb_nr++].match = match;

Then the function is not a mere compute_ws_delta, but a whole
setup_new_pmb(...) thing. Undecided.
Phillip Wood Oct. 3, 2018, 9:42 a.m. UTC | #2 
On 02/10/2018 20:08, 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>
>>
>> Don't duplicate the indentation string if we're not going to use it.
>> This was found with asan.
> 
> Makes sense,
> 
> Thanks,
> Stefan
> 
> With compute_ws_delta growing bigger here (and having only one caller),
> I wonder if we want to pass 'match' instead of match->es (and pmb_nr),
> such that the function can also take care of
>     pmb[pmb_nr++].match = match;

Wouldn't we still need to increment pmb_nr in the caller though, in
which case I'm not sure it saves much.

> Then the function is not a mere compute_ws_delta, but a whole
> setup_new_pmb(...) thing. Undecided.
>
Martin Ågren Oct. 3, 2018, 9:54 a.m. UTC | #3 
On Tue, 2 Oct 2018 at 20:04, Phillip Wood <phillip.wood@talktalk.net> wrote:
>         const struct emitted_diff_symbol *longer =  a->len > b->len ? a : b;
>         const struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;
>         int d = longer->len - shorter->len;
> +       int ret = !strncmp(longer->line + d, shorter->line, shorter->len);
> +
> +       if (!ret)
> +               return ret;
>
>         out->string = xmemdupz(longer->line, d);
>         out->current_longer = (a == longer);
>
> -       return !strncmp(longer->line + d, shorter->line, shorter->len);
> +       return ret;
>  }

The caller only cares about 0 vs non-0, so this would also work:

   if (strncmp(...))
           return 0;

   ...

   return 1;

Just a thought, to avoid some "!"s and the new variable `ret`.

Martin
diff mbox series

Patch

diff --git a/diff.c b/diff.c
index 0096bdc339..efadd05c90 100644
--- a/diff.c
+++ b/diff.c
@@ -785,11 +785,15 @@  static int compute_ws_delta(const struct emitted_diff_symbol *a,
 	const struct emitted_diff_symbol *longer =  a->len > b->len ? a : b;
 	const struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;
 	int d = longer->len - shorter->len;
+	int ret = !strncmp(longer->line + d, shorter->line, shorter->len);
+
+	if (!ret)
+		return ret;
 
 	out->string = xmemdupz(longer->line, d);
 	out->current_longer = (a == longer);
 
-	return !strncmp(longer->line + d, shorter->line, shorter->len);
+	return ret;
 }
 
 static int cmp_in_block_with_wsd(const struct diff_options *o,