{"thread":{"id":"49468","subject":"[PATCH 2/5] diff --color-moved-ws: fix out of bounds string access","startedAt":"2018-10-02T17:55:39Z","lastAt":"2018-10-04T19:42:49Z","messageCount":21,"participants":["Phillip Wood","Stefan Beller","Martin Ågren"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"359436","messageId":"20181002175514.31495-2-phillip.wood@talktalk.net","threadId":"49468","inReplyTo":"20181002175514.31495-1-phillip.wood@talktalk.net","subject":"[PATCH 2/5] diff --color-moved-ws: fix out of bounds string access","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-02T17:55:11Z","receivedAt":"2018-10-02T17:55:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen adjusting the start of the string to take account of the change\nin indentation the code was not checking that the string being\nadjusted was in fact longer than the indentation change. This was\ndetected by asan.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 5a08d64497..0096bdc339 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -841,7 +841,7 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \t\tal -= wslen;\n \t}\n \n-\tif (strcmp(a, c))\n+\tif (al < 0 || al != cl || memcmp(a, c, al))\n \t\treturn 1;\n \n \treturn 0;\n-- \n2.19.0\n\n"},{"id":"359437","messageId":"20181002175514.31495-3-phillip.wood@talktalk.net","threadId":"49468","inReplyTo":"20181002175514.31495-1-phillip.wood@talktalk.net","subject":"[PATCH 3/5] diff --color-moved-ws: fix a memory leak","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-02T17:55:12Z","receivedAt":"2018-10-02T17:55:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nDon't duplicate the indentation string if we're not going to use it.\nThis was found with asan.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n diff.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 0096bdc339..efadd05c90 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -785,11 +785,15 @@ static int compute_ws_delta(const struct emitted_diff_symbol *a,\n \tconst struct emitted_diff_symbol *longer =  a->len > b->len ? a : b;\n \tconst struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;\n \tint d = longer->len - shorter->len;\n+\tint ret = !strncmp(longer->line + d, shorter->line, shorter->len);\n+\n+\tif (!ret)\n+\t\treturn ret;\n \n \tout->string = xmemdupz(longer->line, d);\n \tout->current_longer = (a == longer);\n \n-\treturn !strncmp(longer->line + d, shorter->line, shorter->len);\n+\treturn ret;\n }\n \n static int cmp_in_block_with_wsd(const struct diff_options *o,\n-- \n2.19.0\n\n"},{"id":"359438","messageId":"20181002175514.31495-4-phillip.wood@talktalk.net","threadId":"49468","inReplyTo":"20181002175514.31495-1-phillip.wood@talktalk.net","subject":"[PATCH 4/5] diff --color-moved-ws: fix another memory leak","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-02T17:55:13Z","receivedAt":"2018-10-02T17:55:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nThis is obvious in retrospect, it was found with asan.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n diff.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex efadd05c90..4464feacf8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -971,6 +971,8 @@ static void pmb_advance_or_null_multi_match(struct diff_options *o,\n \t\t\tmoved_block_clear(&pmb[i]);\n \t\t}\n \t}\n+\n+\tfree(got_match);\n }\n \n static int shrink_potential_moved_blocks(struct moved_block *pmb,\n-- \n2.19.0\n\n"},{"id":"359439","messageId":"20181002175514.31495-1-phillip.wood@talktalk.net","threadId":"49468","inReplyTo":null,"subject":"[PATCH 1/5] diff --color-moved-ws: fix double free crash","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-02T17:55:10Z","receivedAt":"2018-10-02T17:55:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nRunning\n  git diff --color-moved-ws=allow-indentation-change v2.18.0 v2.19.0\nresults in a crash due to a double free. This happens when two\npotential moved blocks start with consecutive lines. As\npmb_advance_or_null_multi_match() advances it copies the ws_delta from\nthe last matching line to the next. When the first of our consecutive\nlines is advanced its ws_delta well be copied to the second,\noverwriting the ws_delta of the block containing the second line. Then\nwhen the second line is advanced it will copy the new ws_delta to the\nline below it and so on. Eventually one of these blocks will stop\nmatching and the ws_delta will be freed. From then on the other block\nis in a use-after-free state and when it stops matching it will try to\nfree the ws_delta that has already been freed by the other block.\n\nThe solution is to store the ws_delta in the array of potential moved\nblocks rather than with the lines. This means that it no longer needs\nto be copied around and one block cannot overwrite the ws_delta of\nanother. Additionally it saves some malloc/free calls as we don't keep\nallocating and freeing ws_deltas.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n diff.c | 73 +++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 41 insertions(+), 32 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 9393993e33..5a08d64497 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -751,7 +751,6 @@ struct moved_entry {\n \tstruct hashmap_entry ent;\n \tconst struct emitted_diff_symbol *es;\n \tstruct moved_entry *next_line;\n-\tstruct ws_delta *wsd;\n };\n \n /**\n@@ -768,6 +767,17 @@ struct ws_delta {\n };\n #define WS_DELTA_INIT { NULL, 0 }\n \n+struct moved_block {\n+\tstruct moved_entry *match;\n+\tstruct ws_delta wsd;\n+};\n+\n+static void moved_block_clear(struct moved_block *b)\n+{\n+\tFREE_AND_NULL(b->wsd.string);\n+\tb->match = NULL;\n+}\n+\n static int compute_ws_delta(const struct emitted_diff_symbol *a,\n \t\t\t     const struct emitted_diff_symbol *b,\n \t\t\t     struct ws_delta *out)\n@@ -785,7 +795,7 @@ static int compute_ws_delta(const struct emitted_diff_symbol *a,\n static int cmp_in_block_with_wsd(const struct diff_options *o,\n \t\t\t\t const struct moved_entry *cur,\n \t\t\t\t const struct moved_entry *match,\n-\t\t\t\t struct moved_entry *pmb,\n+\t\t\t\t struct moved_block *pmb,\n \t\t\t\t int n)\n {\n \tstruct emitted_diff_symbol *l = &o->emitted_symbols->buf[n];\n@@ -805,7 +815,7 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \tif (strcmp(a, b))\n \t\treturn 1;\n \n-\tif (!pmb->wsd)\n+\tif (!pmb->wsd.string)\n \t\t/*\n \t\t * No white space delta was carried forward? This can happen\n \t\t * when we exit early in this function and do not carry\n@@ -822,8 +832,8 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \t * one of them for the white spaces, depending which was longer.\n \t */\n \n-\twslen = strlen(pmb->wsd->string);\n-\tif (pmb->wsd->current_longer) {\n+\twslen = strlen(pmb->wsd.string);\n+\tif (pmb->wsd.current_longer) {\n \t\tc += wslen;\n \t\tcl -= wslen;\n \t} else {\n@@ -873,7 +883,6 @@ static struct moved_entry *prepare_entry(struct diff_options *o,\n \tret->ent.hash = xdiff_hash_string(l->line, l->len, flags);\n \tret->es = l;\n \tret->next_line = NULL;\n-\tret->wsd = NULL;\n \n \treturn ret;\n }\n@@ -913,76 +922,72 @@ static void add_lines_to_move_detection(struct diff_options *o,\n static void pmb_advance_or_null(struct diff_options *o,\n \t\t\t\tstruct moved_entry *match,\n \t\t\t\tstruct hashmap *hm,\n-\t\t\t\tstruct moved_entry **pmb,\n+\t\t\t\tstruct moved_block *pmb,\n \t\t\t\tint pmb_nr)\n {\n \tint i;\n \tfor (i = 0; i < pmb_nr; i++) {\n-\t\tstruct moved_entry *prev = pmb[i];\n+\t\tstruct moved_entry *prev = pmb[i].match;\n \t\tstruct moved_entry *cur = (prev && prev->next_line) ?\n \t\t\t\tprev->next_line : NULL;\n \t\tif (cur && !hm->cmpfn(o, cur, match, NULL)) {\n-\t\t\tpmb[i] = cur;\n+\t\t\tpmb[i].match = cur;\n \t\t} else {\n-\t\t\tpmb[i] = NULL;\n+\t\t\tpmb[i].match = NULL;\n \t\t}\n \t}\n }\n \n static void pmb_advance_or_null_multi_match(struct diff_options *o,\n \t\t\t\t\t    struct moved_entry *match,\n \t\t\t\t\t    struct hashmap *hm,\n-\t\t\t\t\t    struct moved_entry **pmb,\n+\t\t\t\t\t    struct moved_block *pmb,\n \t\t\t\t\t    int pmb_nr, int n)\n {\n \tint i;\n \tchar *got_match = xcalloc(1, pmb_nr);\n \n \tfor (; match; match = hashmap_get_next(hm, match)) {\n \t\tfor (i = 0; i < pmb_nr; i++) {\n-\t\t\tstruct moved_entry *prev = pmb[i];\n+\t\t\tstruct moved_entry *prev = pmb[i].match;\n \t\t\tstruct moved_entry *cur = (prev && prev->next_line) ?\n \t\t\t\t\tprev->next_line : NULL;\n \t\t\tif (!cur)\n \t\t\t\tcontinue;\n-\t\t\tif (!cmp_in_block_with_wsd(o, cur, match, pmb[i], n))\n+\t\t\tif (!cmp_in_block_with_wsd(o, cur, match, &pmb[i], n))\n \t\t\t\tgot_match[i] |= 1;\n \t\t}\n \t}\n \n \tfor (i = 0; i < pmb_nr; i++) {\n \t\tif (got_match[i]) {\n \t\t\t/* Carry the white space delta forward */\n-\t\t\tpmb[i]->next_line->wsd = pmb[i]->wsd;\n-\t\t\tpmb[i] = pmb[i]->next_line;\n+\t\t\tpmb[i].match = pmb[i].match->next_line;\n \t\t} else {\n-\t\t\tif (pmb[i]->wsd) {\n-\t\t\t\tfree(pmb[i]->wsd->string);\n-\t\t\t\tFREE_AND_NULL(pmb[i]->wsd);\n-\t\t\t}\n-\t\t\tpmb[i] = NULL;\n+\t\t\tmoved_block_clear(&pmb[i]);\n \t\t}\n \t}\n }\n \n-static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n+static int shrink_potential_moved_blocks(struct moved_block *pmb,\n \t\t\t\t\t int pmb_nr)\n {\n \tint lp, rp;\n \n \t/* Shrink the set of potential block to the remaining running */\n \tfor (lp = 0, rp = pmb_nr - 1; lp <= rp;) {\n-\t\twhile (lp < pmb_nr && pmb[lp])\n+\t\twhile (lp < pmb_nr && pmb[lp].match)\n \t\t\tlp++;\n \t\t/* lp points at the first NULL now */\n \n-\t\twhile (rp > -1 && !pmb[rp])\n+\t\twhile (rp > -1 && !pmb[rp].match)\n \t\t\trp--;\n \t\t/* rp points at the last non-NULL */\n \n \t\tif (lp < pmb_nr && rp > -1 && lp < rp) {\n \t\t\tpmb[lp] = pmb[rp];\n-\t\t\tpmb[rp] = NULL;\n+\t\t\tpmb[rp].match = NULL;\n+\t\t\tpmb[rp].wsd.string = NULL;\n \t\t\trp--;\n \t\t\tlp++;\n \t\t}\n@@ -1029,7 +1034,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\tstruct hashmap *add_lines,\n \t\t\t\tstruct hashmap *del_lines)\n {\n-\tstruct moved_entry **pmb = NULL; /* potentially moved blocks */\n+\tstruct moved_block *pmb = NULL; /* potentially moved blocks */\n \tint pmb_nr = 0, pmb_alloc = 0;\n \tint n, flipped_block = 1, block_length = 0;\n \n@@ -1058,7 +1063,11 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t}\n \n \t\tif (!match) {\n+\t\t\tint i;\n+\n \t\t\tadjust_last_block(o, n, block_length);\n+\t\t\tfor(i = 0; i < pmb_nr; i++)\n+\t\t\t\tmoved_block_clear(&pmb[i]);\n \t\t\tpmb_nr = 0;\n \t\t\tblock_length = 0;\n \t\t\tcontinue;\n@@ -1086,14 +1095,12 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\tALLOC_GROW(pmb, pmb_nr + 1, pmb_alloc);\n \t\t\t\tif (o->color_moved_ws_handling &\n \t\t\t\t    COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) {\n-\t\t\t\t\tstruct ws_delta *wsd = xmalloc(sizeof(*match->wsd));\n-\t\t\t\t\tif (compute_ws_delta(l, match->es, wsd)) {\n-\t\t\t\t\t\tmatch->wsd = wsd;\n-\t\t\t\t\t\tpmb[pmb_nr++] = match;\n-\t\t\t\t\t} else\n-\t\t\t\t\t\tfree(wsd);\n+\t\t\t\t\tif (compute_ws_delta(l, match->es,\n+\t\t\t\t\t\t\t     &pmb[pmb_nr].wsd))\n+\t\t\t\t\t\tpmb[pmb_nr++].match = match;\n \t\t\t\t} else {\n-\t\t\t\t\tpmb[pmb_nr++] = match;\n+\t\t\t\t\tpmb[pmb_nr].wsd.string = NULL;\n+\t\t\t\t\tpmb[pmb_nr++].match = match;\n \t\t\t\t}\n \t\t\t}\n \n@@ -1110,6 +1117,8 @@ static void mark_color_as_moved(struct diff_options *o,\n \t}\n \tadjust_last_block(o, n, block_length);\n \n+\tfor(n = 0; n < pmb_nr; n++)\n+\t\tmoved_block_clear(&pmb[n]);\n \tfree(pmb);\n }\n \n-- \n2.19.0\n\n"},{"id":"359440","messageId":"20181002175514.31495-5-phillip.wood@talktalk.net","threadId":"49468","inReplyTo":"20181002175514.31495-1-phillip.wood@talktalk.net","subject":"[PATCH 5/5] diff --color-moved: fix a memory leak","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-02T17:55:14Z","receivedAt":"2018-10-02T17:55:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nFree the hashmap items as well as the hashmap itself. This was found\nwith asan.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 4464feacf8..94cc1b5592 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5770,8 +5770,8 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n \t\t\tif (o->color_moved == COLOR_MOVED_ZEBRA_DIM)\n \t\t\t\tdim_moved_lines(o);\n \n-\t\t\thashmap_free(&add_lines, 0);\n-\t\t\thashmap_free(&del_lines, 0);\n+\t\t\thashmap_free(&add_lines, 1);\n+\t\t\thashmap_free(&del_lines, 1);\n \t\t}\n \n \t\tfor (i = 0; i < esm.nr; i++)\n-- \n2.19.0\n\n"},{"id":"359443","messageId":"CAGZ79kYqACgoNShaPBQQSUN1B_dhmdxHxBV6YZoNAd39f6XMjw@mail.gmail.com","threadId":"49468","inReplyTo":"20181002175514.31495-1-phillip.wood@talktalk.net","subject":"Re: [PATCH 1/5] diff --color-moved-ws: fix double free crash","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-02T18:49:28Z","receivedAt":"2018-10-02T18:49:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood <phillip.wood@talktalk.net> wrote:\n\n> The solution is to store the ws_delta in the array of potential moved\n> blocks rather than with the lines. This means that it no longer needs\n> to be copied around and one block cannot overwrite the ws_delta of\n> another. Additionally it saves some malloc/free calls as we don't keep\n> allocating and freeing ws_deltas.\n\nAnother solution would be to duplicate the copy-arounds, that it only\nfixes the double free, but having another layer of abstraction\n(moved block vs line) makes sense as then we don't need to copy\nit forward.\n\nWith this patch applied the diff as mentioned works and having the\nws deltas with the blocks instead of the\n\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n\n\n\n>  static void pmb_advance_or_null_multi_match(struct diff_options *o,\n[...]\n>         for (i = 0; i < pmb_nr; i++) {\n>                 if (got_match[i]) {\n>                         /* Carry the white space delta forward */\n\nI would think this comment is obsolete as well with this patch?\n\nWith or without that nit addressed, this patch is\nReviewed-by: Stefan Beller <sbeller@google.com>\n"},{"id":"359444","messageId":"CAGZ79kYu9enUT=yL8wfM9quYYz1fs5OU4Wcvci9ZVPPdXkY6gA@mail.gmail.com","threadId":"49468","inReplyTo":"20181002175514.31495-2-phillip.wood@talktalk.net","subject":"Re: [PATCH 2/5] diff --color-moved-ws: fix out of bounds string access","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-02T18:58:45Z","receivedAt":"2018-10-02T18:59:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood <phillip.wood@talktalk.net> wrote:\n>\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> When adjusting the start of the string to take account of the change\n> in indentation the code was not checking that the string being\n> adjusted was in fact longer than the indentation change. This was\n> detected by asan.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  diff.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 5a08d64497..0096bdc339 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -841,7 +841,7 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n>                 al -= wslen;\n>         }\n>\n> -       if (strcmp(a, c))\n> +       if (al < 0 || al != cl || memcmp(a, c, al))\n\nIf (al < 0) then al != cl, so I would think we could drop the first\npart, and only check with al != cl || memcmp\n\nIn theory we should use xdiff_compare_lines here\nas the rest of the lines could have more white space issues,\nbut we catch that earlier via a die(\"color-moved-ws:\nallow-indentation-change cannot be combined with other\nwhite space modes\"), so memcmp is fine.\n\nSide note: There are comments above this code that seem\nto be also obsolete after patch 1 (\"...carried forward in\npmb->wsd; ...)\n"},{"id":"359446","messageId":"CAGZ79kbX+DssZaCS_fV-eNR828Yd-vsLJ6F58Lifn2f8j+oXvA@mail.gmail.com","threadId":"49468","inReplyTo":"20181002175514.31495-3-phillip.wood@talktalk.net","subject":"Re: [PATCH 3/5] diff --color-moved-ws: fix a memory leak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-02T19:08:02Z","receivedAt":"2018-10-02T19:08:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood <phillip.wood@talktalk.net> wrote:\n>\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Don't duplicate the indentation string if we're not going to use it.\n> This was found with asan.\n\nMakes sense,\n\nThanks,\nStefan\n\nWith compute_ws_delta growing bigger here (and having only one caller),\nI wonder if we want to pass 'match' instead of match->es (and pmb_nr),\nsuch that the function can also take care of\n    pmb[pmb_nr++].match = match;\n\nThen the function is not a mere compute_ws_delta, but a whole\nsetup_new_pmb(...) thing. Undecided.\n"},{"id":"359447","messageId":"CAGZ79kasT3ygWGArL5t+O4modkDG7AwzXmpf7vi1TdkZBTh1Uw@mail.gmail.com","threadId":"49468","inReplyTo":"20181002175514.31495-4-phillip.wood@talktalk.net","subject":"Re: [PATCH 4/5] diff --color-moved-ws: fix another memory leak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-02T19:08:49Z","receivedAt":"2018-10-02T19:09:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood <phillip.wood@talktalk.net> wrote:\n>\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> This is obvious in retrospect, it was found with asan.\n\nIndeed. :/\n\nThanks,\nStefan\n"},{"id":"359450","messageId":"CAGZ79kZDEeBZbTC=yb3CcJz54GM4jn_669H3T1fiNXs9Oob7aA@mail.gmail.com","threadId":"49468","inReplyTo":"20181002175514.31495-5-phillip.wood@talktalk.net","subject":"Re: [PATCH 5/5] diff --color-moved: fix a memory leak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-02T19:11:58Z","receivedAt":"2018-10-02T19:12:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood <phillip.wood@talktalk.net> wrote:\n>\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Free the hashmap items as well as the hashmap itself. This was found\n> with asan.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  diff.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 4464feacf8..94cc1b5592 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -5770,8 +5770,8 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n>                         if (o->color_moved == COLOR_MOVED_ZEBRA_DIM)\n>                                 dim_moved_lines(o);\n>\n> -                       hashmap_free(&add_lines, 0);\n> -                       hashmap_free(&del_lines, 0);\n> +                       hashmap_free(&add_lines, 1);\n> +                       hashmap_free(&del_lines, 1);\n\nAnd this is sufficient as the entries themselves are\nmoved_entry, which only contain pointers (to\no->emitted_symbols), which are freed later.\nThere is nothing in there that needs extra free-ing.\n\nThanks,\nStefan\n"},{"id":"359502","messageId":"1c6b26a2-45ca-df76-8baa-61790cb963ac@talktalk.net","threadId":"49468","inReplyTo":"CAGZ79kYqACgoNShaPBQQSUN1B_dhmdxHxBV6YZoNAd39f6XMjw@mail.gmail.com","subject":"Re: [PATCH 1/5] diff --color-moved-ws: fix double free crash","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-03T09:38:48Z","receivedAt":"2018-10-03T09:38:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stefan\n\nThanks for looking at these patches\n\nOn 02/10/2018 19:49, Stefan Beller wrote:\n> On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood <phillip.wood@talktalk.net> wrote:\n> \n>> The solution is to store the ws_delta in the array of potential moved\n>> blocks rather than with the lines. This means that it no longer needs\n>> to be copied around and one block cannot overwrite the ws_delta of\n>> another. Additionally it saves some malloc/free calls as we don't keep\n>> allocating and freeing ws_deltas.\n> \n> Another solution would be to duplicate the copy-arounds, that it only\n> fixes the double free, but having another layer of abstraction\n> (moved block vs line) makes sense as then we don't need to copy\n> it forward.\n> \n> With this patch applied the diff as mentioned works and having the\n> ws deltas with the blocks instead of the\n> \n>>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n> \n> \n>>  static void pmb_advance_or_null_multi_match(struct diff_options *o,\n> [...]\n>>         for (i = 0; i < pmb_nr; i++) {\n>>                 if (got_match[i]) {\n>>                         /* Carry the white space delta forward */\n> \n> I would think this comment is obsolete as well with this patch?\n\nYes you're right I should have removed that. As there are some changes\nneeded to some other comments I'll re-roll\n\nBest Wishes\n\nPhillip\n\n> \n> With or without that nit addressed, this patch is\n> Reviewed-by: Stefan Beller <sbeller@google.com>\n> \n\n"},{"id":"359503","messageId":"ea197dba-bb51-0dde-bd2b-84709ce81a51@talktalk.net","threadId":"49468","inReplyTo":"CAGZ79kYu9enUT=yL8wfM9quYYz1fs5OU4Wcvci9ZVPPdXkY6gA@mail.gmail.com","subject":"Re: [PATCH 2/5] diff --color-moved-ws: fix out of bounds string access","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-03T09:40:32Z","receivedAt":"2018-10-03T09:40:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 02/10/2018 19:58, Stefan Beller wrote:\n> On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood <phillip.wood@talktalk.net> wrote:\n>>\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> When adjusting the start of the string to take account of the change\n>> in indentation the code was not checking that the string being\n>> adjusted was in fact longer than the indentation change. This was\n>> detected by asan.\n>>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> ---\n>>  diff.c | 2 +-\n>>  1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/diff.c b/diff.c\n>> index 5a08d64497..0096bdc339 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -841,7 +841,7 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n>>                 al -= wslen;\n>>         }\n>>\n>> -       if (strcmp(a, c))\n>> +       if (al < 0 || al != cl || memcmp(a, c, al))\n> \n> If (al < 0) then al != cl, so I would think we could drop the first\n> part, and only check with al != cl || memcmp\n\nYes, I couldn't make up my mind weather to add the al < 0 or not but of\ncourse only one of the lengths can ever be negative, I'll remove it\n\n> In theory we should use xdiff_compare_lines here\n> as the rest of the lines could have more white space issues,\n> but we catch that earlier via a die(\"color-moved-ws:\n> allow-indentation-change cannot be combined with other\n> white space modes\"), so memcmp is fine.\n> \n> Side note: There are comments above this code that seem\n> to be also obsolete after patch 1 (\"...carried forward in\n> pmb->wsd; ...)\n\nGood point I'll go through the comments and update them\n\nBest Wishes\n\nPhillip\n\n"},{"id":"359504","messageId":"5821aa37-b1aa-9118-5920-cfa60b243032@talktalk.net","threadId":"49468","inReplyTo":"CAGZ79kbX+DssZaCS_fV-eNR828Yd-vsLJ6F58Lifn2f8j+oXvA@mail.gmail.com","subject":"Re: [PATCH 3/5] diff --color-moved-ws: fix a memory leak","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-03T09:42:14Z","receivedAt":"2018-10-03T09:42:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 02/10/2018 20:08, Stefan Beller wrote:\n> On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood <phillip.wood@talktalk.net> wrote:\n>>\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> Don't duplicate the indentation string if we're not going to use it.\n>> This was found with asan.\n> \n> Makes sense,\n> \n> Thanks,\n> Stefan\n> \n> With compute_ws_delta growing bigger here (and having only one caller),\n> I wonder if we want to pass 'match' instead of match->es (and pmb_nr),\n> such that the function can also take care of\n>     pmb[pmb_nr++].match = match;\n\nWouldn't we still need to increment pmb_nr in the caller though, in\nwhich case I'm not sure it saves much.\n\n> Then the function is not a mere compute_ws_delta, but a whole\n> setup_new_pmb(...) thing. Undecided.\n> \n\n"},{"id":"359505","messageId":"CAN0heSpF_5WSHWWwFYnQYWhwMopjOSB8JvKh2DYhRORjDN_jdA@mail.gmail.com","threadId":"49468","inReplyTo":"20181002175514.31495-3-phillip.wood@talktalk.net","subject":"Re: [PATCH 3/5] diff --color-moved-ws: fix a memory leak","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-10-03T09:54:52Z","receivedAt":"2018-10-03T09:55:12Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 2 Oct 2018 at 20:04, Phillip Wood <phillip.wood@talktalk.net> wrote:\n>         const struct emitted_diff_symbol *longer =  a->len > b->len ? a : b;\n>         const struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;\n>         int d = longer->len - shorter->len;\n> +       int ret = !strncmp(longer->line + d, shorter->line, shorter->len);\n> +\n> +       if (!ret)\n> +               return ret;\n>\n>         out->string = xmemdupz(longer->line, d);\n>         out->current_longer = (a == longer);\n>\n> -       return !strncmp(longer->line + d, shorter->line, shorter->len);\n> +       return ret;\n>  }\n\nThe caller only cares about 0 vs non-0, so this would also work:\n\n   if (strncmp(...))\n           return 0;\n\n   ...\n\n   return 1;\n\nJust a thought, to avoid some \"!\"s and the new variable `ret`.\n\nMartin\n"},{"id":"359610","messageId":"20181004100745.4568-1-phillip.wood@talktalk.net","threadId":"49468","inReplyTo":"20181002175514.31495-1-phillip.wood@talktalk.net","subject":"[PATCH v2 0/5] diff --color-moved-ws: fix double free crash","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-04T10:07:40Z","receivedAt":"2018-10-04T10:07:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nThanks to Stefan and Martin for their comments on v1. This version has\nsome small changes to patches 1-3 based on that feedback - see the\nindividual patches for what has changed.\n\nPhillip Wood (5):\n  diff --color-moved-ws: fix double free crash\n  diff --color-moved-ws: fix out of bounds string access\n  diff --color-moved-ws: fix a memory leak\n  diff --color-moved-ws: fix another memory leak\n  diff --color-moved: fix a memory leak\n\n diff.c | 95 +++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 54 insertions(+), 41 deletions(-)\n\n-- \n2.19.0\n\n"},{"id":"359611","messageId":"20181004100745.4568-3-phillip.wood@talktalk.net","threadId":"49468","inReplyTo":"20181004100745.4568-1-phillip.wood@talktalk.net","subject":"[PATCH v2 2/5] diff --color-moved-ws: fix out of bounds string access","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-04T10:07:42Z","receivedAt":"2018-10-04T10:07:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen adjusting the start of the string to take account of the change\nin indentation the code was not checking that the string being\nadjusted was in fact longer than the indentation change. This was\ndetected by asan.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n\nNotes:\n    Changes since v1:\n     - simplified comparison as suggested by Stefan\n\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 02d885f039..e492f8b74f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -840,7 +840,7 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \t\tal -= wslen;\n \t}\n \n-\tif (strcmp(a, c))\n+\tif (al != cl || memcmp(a, c, al))\n \t\treturn 1;\n \n \treturn 0;\n-- \n2.19.0\n\n"},{"id":"359612","messageId":"20181004100745.4568-4-phillip.wood@talktalk.net","threadId":"49468","inReplyTo":"20181004100745.4568-1-phillip.wood@talktalk.net","subject":"[PATCH v2 3/5] diff --color-moved-ws: fix a memory leak","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-04T10:07:43Z","receivedAt":"2018-10-04T10:07:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nDon't duplicate the indentation string if we're not going to use it.\nThis was found with asan.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n\nNotes:\n    Changes since v1:\n     - simplified code as suggested by Martin Ågren\n\n diff.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex e492f8b74f..9bf41bad0d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -786,10 +786,13 @@ static int compute_ws_delta(const struct emitted_diff_symbol *a,\n \tconst struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;\n \tint d = longer->len - shorter->len;\n \n+\tif (strncmp(longer->line + d, shorter->line, shorter->len))\n+\t\treturn 0;\n+\n \tout->string = xmemdupz(longer->line, d);\n \tout->current_longer = (a == longer);\n \n-\treturn !strncmp(longer->line + d, shorter->line, shorter->len);\n+\treturn 1;\n }\n \n static int cmp_in_block_with_wsd(const struct diff_options *o,\n-- \n2.19.0\n\n"},{"id":"359613","messageId":"20181004100745.4568-6-phillip.wood@talktalk.net","threadId":"49468","inReplyTo":"20181004100745.4568-1-phillip.wood@talktalk.net","subject":"[PATCH v2 5/5] diff --color-moved: fix a memory leak","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-04T10:07:45Z","receivedAt":"2018-10-04T10:08:01Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nFree the hashmap items as well as the hashmap itself. This was found\nwith asan.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 69f6309db6..100d24b9c4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5768,8 +5768,8 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n \t\t\tif (o->color_moved == COLOR_MOVED_ZEBRA_DIM)\n \t\t\t\tdim_moved_lines(o);\n \n-\t\t\thashmap_free(&add_lines, 0);\n-\t\t\thashmap_free(&del_lines, 0);\n+\t\t\thashmap_free(&add_lines, 1);\n+\t\t\thashmap_free(&del_lines, 1);\n \t\t}\n \n \t\tfor (i = 0; i < esm.nr; i++)\n-- \n2.19.0\n\n"},{"id":"359614","messageId":"20181004100745.4568-5-phillip.wood@talktalk.net","threadId":"49468","inReplyTo":"20181004100745.4568-1-phillip.wood@talktalk.net","subject":"[PATCH v2 4/5] diff --color-moved-ws: fix another memory leak","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-04T10:07:44Z","receivedAt":"2018-10-04T10:08:02Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nThis is obvious in retrospect, it was found with asan.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n diff.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 9bf41bad0d..69f6309db6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -969,6 +969,8 @@ static void pmb_advance_or_null_multi_match(struct diff_options *o,\n \t\t\tmoved_block_clear(&pmb[i]);\n \t\t}\n \t}\n+\n+\tfree(got_match);\n }\n \n static int shrink_potential_moved_blocks(struct moved_block *pmb,\n-- \n2.19.0\n\n"},{"id":"359615","messageId":"20181004100745.4568-2-phillip.wood@talktalk.net","threadId":"49468","inReplyTo":"20181004100745.4568-1-phillip.wood@talktalk.net","subject":"[PATCH v2 1/5] diff --color-moved-ws: fix double free crash","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-04T10:07:41Z","receivedAt":"2018-10-04T10:08:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nRunning\n  git diff --color-moved-ws=allow-indentation-change v2.18.0 v2.19.0\nresults in a crash due to a double free. This happens when two\npotential moved blocks start with consecutive lines. As\npmb_advance_or_null_multi_match() advances it copies the ws_delta from\nthe last matching line to the next. When the first of our consecutive\nlines is advanced its ws_delta well be copied to the second,\noverwriting the ws_delta of the block containing the second line. Then\nwhen the second line is advanced it will copy the new ws_delta to the\nline below it and so on. Eventually one of these blocks will stop\nmatching and the ws_delta will be freed. From then on the other block\nis in a use-after-free state and when it stops matching it will try to\nfree the ws_delta that has already been freed by the other block.\n\nThe solution is to store the ws_delta in the array of potential moved\nblocks rather than with the lines. This means that it no longer needs\nto be copied around and one block cannot overwrite the ws_delta of\nanother. Additionally it saves some malloc/free calls as we don't keep\nallocating and freeing ws_deltas.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n\nNotes:\n    This applies on top of fab01ec52e (\"diff: fix\n    --color-moved-ws=allow-indentation-change\", 2018-09-04). Note that the\n    crash happens with or without that patch, but the mechanism is\n    slightly different without it.\n    \n    Changes since v1:\n     - updated comments to match new implementation.\n\n diff.c | 82 ++++++++++++++++++++++++++++++++--------------------------\n 1 file changed, 45 insertions(+), 37 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 9393993e33..02d885f039 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -751,7 +751,6 @@ struct moved_entry {\n \tstruct hashmap_entry ent;\n \tconst struct emitted_diff_symbol *es;\n \tstruct moved_entry *next_line;\n-\tstruct ws_delta *wsd;\n };\n \n /**\n@@ -768,6 +767,17 @@ struct ws_delta {\n };\n #define WS_DELTA_INIT { NULL, 0 }\n \n+struct moved_block {\n+\tstruct moved_entry *match;\n+\tstruct ws_delta wsd;\n+};\n+\n+static void moved_block_clear(struct moved_block *b)\n+{\n+\tFREE_AND_NULL(b->wsd.string);\n+\tb->match = NULL;\n+}\n+\n static int compute_ws_delta(const struct emitted_diff_symbol *a,\n \t\t\t     const struct emitted_diff_symbol *b,\n \t\t\t     struct ws_delta *out)\n@@ -785,7 +795,7 @@ static int compute_ws_delta(const struct emitted_diff_symbol *a,\n static int cmp_in_block_with_wsd(const struct diff_options *o,\n \t\t\t\t const struct moved_entry *cur,\n \t\t\t\t const struct moved_entry *match,\n-\t\t\t\t struct moved_entry *pmb,\n+\t\t\t\t struct moved_block *pmb,\n \t\t\t\t int n)\n {\n \tstruct emitted_diff_symbol *l = &o->emitted_symbols->buf[n];\n@@ -805,25 +815,24 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \tif (strcmp(a, b))\n \t\treturn 1;\n \n-\tif (!pmb->wsd)\n+\tif (!pmb->wsd.string)\n \t\t/*\n-\t\t * No white space delta was carried forward? This can happen\n-\t\t * when we exit early in this function and do not carry\n-\t\t * forward ws.\n+\t\t * The white space delta is not active? This can happen\n+\t\t * when we exit early in this function.\n \t\t */\n \t\treturn 1;\n \n \t/*\n-\t * The indent changes of the block are known and carried forward in\n+\t * The indent changes of the block are known and stored in\n \t * pmb->wsd; however we need to check if the indent changes of the\n \t * current line are still the same as before.\n \t *\n \t * To do so we need to compare 'l' to 'cur', adjusting the\n \t * one of them for the white spaces, depending which was longer.\n \t */\n \n-\twslen = strlen(pmb->wsd->string);\n-\tif (pmb->wsd->current_longer) {\n+\twslen = strlen(pmb->wsd.string);\n+\tif (pmb->wsd.current_longer) {\n \t\tc += wslen;\n \t\tcl -= wslen;\n \t} else {\n@@ -873,7 +882,6 @@ static struct moved_entry *prepare_entry(struct diff_options *o,\n \tret->ent.hash = xdiff_hash_string(l->line, l->len, flags);\n \tret->es = l;\n \tret->next_line = NULL;\n-\tret->wsd = NULL;\n \n \treturn ret;\n }\n@@ -913,76 +921,72 @@ static void add_lines_to_move_detection(struct diff_options *o,\n static void pmb_advance_or_null(struct diff_options *o,\n \t\t\t\tstruct moved_entry *match,\n \t\t\t\tstruct hashmap *hm,\n-\t\t\t\tstruct moved_entry **pmb,\n+\t\t\t\tstruct moved_block *pmb,\n \t\t\t\tint pmb_nr)\n {\n \tint i;\n \tfor (i = 0; i < pmb_nr; i++) {\n-\t\tstruct moved_entry *prev = pmb[i];\n+\t\tstruct moved_entry *prev = pmb[i].match;\n \t\tstruct moved_entry *cur = (prev && prev->next_line) ?\n \t\t\t\tprev->next_line : NULL;\n \t\tif (cur && !hm->cmpfn(o, cur, match, NULL)) {\n-\t\t\tpmb[i] = cur;\n+\t\t\tpmb[i].match = cur;\n \t\t} else {\n-\t\t\tpmb[i] = NULL;\n+\t\t\tpmb[i].match = NULL;\n \t\t}\n \t}\n }\n \n static void pmb_advance_or_null_multi_match(struct diff_options *o,\n \t\t\t\t\t    struct moved_entry *match,\n \t\t\t\t\t    struct hashmap *hm,\n-\t\t\t\t\t    struct moved_entry **pmb,\n+\t\t\t\t\t    struct moved_block *pmb,\n \t\t\t\t\t    int pmb_nr, int n)\n {\n \tint i;\n \tchar *got_match = xcalloc(1, pmb_nr);\n \n \tfor (; match; match = hashmap_get_next(hm, match)) {\n \t\tfor (i = 0; i < pmb_nr; i++) {\n-\t\t\tstruct moved_entry *prev = pmb[i];\n+\t\t\tstruct moved_entry *prev = pmb[i].match;\n \t\t\tstruct moved_entry *cur = (prev && prev->next_line) ?\n \t\t\t\t\tprev->next_line : NULL;\n \t\t\tif (!cur)\n \t\t\t\tcontinue;\n-\t\t\tif (!cmp_in_block_with_wsd(o, cur, match, pmb[i], n))\n+\t\t\tif (!cmp_in_block_with_wsd(o, cur, match, &pmb[i], n))\n \t\t\t\tgot_match[i] |= 1;\n \t\t}\n \t}\n \n \tfor (i = 0; i < pmb_nr; i++) {\n \t\tif (got_match[i]) {\n-\t\t\t/* Carry the white space delta forward */\n-\t\t\tpmb[i]->next_line->wsd = pmb[i]->wsd;\n-\t\t\tpmb[i] = pmb[i]->next_line;\n+\t\t\t/* Advance to the next line */\n+\t\t\tpmb[i].match = pmb[i].match->next_line;\n \t\t} else {\n-\t\t\tif (pmb[i]->wsd) {\n-\t\t\t\tfree(pmb[i]->wsd->string);\n-\t\t\t\tFREE_AND_NULL(pmb[i]->wsd);\n-\t\t\t}\n-\t\t\tpmb[i] = NULL;\n+\t\t\tmoved_block_clear(&pmb[i]);\n \t\t}\n \t}\n }\n \n-static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n+static int shrink_potential_moved_blocks(struct moved_block *pmb,\n \t\t\t\t\t int pmb_nr)\n {\n \tint lp, rp;\n \n \t/* Shrink the set of potential block to the remaining running */\n \tfor (lp = 0, rp = pmb_nr - 1; lp <= rp;) {\n-\t\twhile (lp < pmb_nr && pmb[lp])\n+\t\twhile (lp < pmb_nr && pmb[lp].match)\n \t\t\tlp++;\n \t\t/* lp points at the first NULL now */\n \n-\t\twhile (rp > -1 && !pmb[rp])\n+\t\twhile (rp > -1 && !pmb[rp].match)\n \t\t\trp--;\n \t\t/* rp points at the last non-NULL */\n \n \t\tif (lp < pmb_nr && rp > -1 && lp < rp) {\n \t\t\tpmb[lp] = pmb[rp];\n-\t\t\tpmb[rp] = NULL;\n+\t\t\tpmb[rp].match = NULL;\n+\t\t\tpmb[rp].wsd.string = NULL;\n \t\t\trp--;\n \t\t\tlp++;\n \t\t}\n@@ -1029,7 +1033,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\tstruct hashmap *add_lines,\n \t\t\t\tstruct hashmap *del_lines)\n {\n-\tstruct moved_entry **pmb = NULL; /* potentially moved blocks */\n+\tstruct moved_block *pmb = NULL; /* potentially moved blocks */\n \tint pmb_nr = 0, pmb_alloc = 0;\n \tint n, flipped_block = 1, block_length = 0;\n \n@@ -1058,7 +1062,11 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t}\n \n \t\tif (!match) {\n+\t\t\tint i;\n+\n \t\t\tadjust_last_block(o, n, block_length);\n+\t\t\tfor(i = 0; i < pmb_nr; i++)\n+\t\t\t\tmoved_block_clear(&pmb[i]);\n \t\t\tpmb_nr = 0;\n \t\t\tblock_length = 0;\n \t\t\tcontinue;\n@@ -1086,14 +1094,12 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\tALLOC_GROW(pmb, pmb_nr + 1, pmb_alloc);\n \t\t\t\tif (o->color_moved_ws_handling &\n \t\t\t\t    COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) {\n-\t\t\t\t\tstruct ws_delta *wsd = xmalloc(sizeof(*match->wsd));\n-\t\t\t\t\tif (compute_ws_delta(l, match->es, wsd)) {\n-\t\t\t\t\t\tmatch->wsd = wsd;\n-\t\t\t\t\t\tpmb[pmb_nr++] = match;\n-\t\t\t\t\t} else\n-\t\t\t\t\t\tfree(wsd);\n+\t\t\t\t\tif (compute_ws_delta(l, match->es,\n+\t\t\t\t\t\t\t     &pmb[pmb_nr].wsd))\n+\t\t\t\t\t\tpmb[pmb_nr++].match = match;\n \t\t\t\t} else {\n-\t\t\t\t\tpmb[pmb_nr++] = match;\n+\t\t\t\t\tpmb[pmb_nr].wsd.string = NULL;\n+\t\t\t\t\tpmb[pmb_nr++].match = match;\n \t\t\t\t}\n \t\t\t}\n \n@@ -1110,6 +1116,8 @@ static void mark_color_as_moved(struct diff_options *o,\n \t}\n \tadjust_last_block(o, n, block_length);\n \n+\tfor(n = 0; n < pmb_nr; n++)\n+\t\tmoved_block_clear(&pmb[n]);\n \tfree(pmb);\n }\n \n-- \n2.19.0\n\n"},{"id":"359633","messageId":"CAGZ79kbamUK=d+-ejy9vopDiVZF7OVOngz1Zx9y04VR3HnmoXg@mail.gmail.com","threadId":"49468","inReplyTo":"20181004100745.4568-6-phillip.wood@talktalk.net","subject":"Re: [PATCH v2 5/5] diff --color-moved: fix a memory leak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-04T19:42:34Z","receivedAt":"2018-10-04T19:42:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 4, 2018 at 3:07 AM Phillip Wood <phillip.wood@talktalk.net> wrote:\n>\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Free the hashmap items as well as the hashmap itself. This was found\n> with asan.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nThanks for this series, all 5 patches are\n\nReviewed-by: Stefan Beller <sbeller@google.com>\n"}]}