{"thread":{"id":"51459","subject":"[PATCH] ref-filter: fix memory leak in `free_array_item()`","startedAt":"2019-07-10T18:37:32Z","lastAt":"2019-07-10T20:41:25Z","messageCount":2,"participants":["Martin Ågren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"378770","messageId":"20190710183639.4123992-1-martin.agren@gmail.com","threadId":"51459","inReplyTo":null,"subject":"[PATCH] ref-filter: fix memory leak in `free_array_item()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-07-10T18:36:39Z","receivedAt":"2019-07-10T18:37:32Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We treat the `value` pointer as a pointer to a struct and free its `s`\nfield. But `value` is in fact an array of structs. As a result, we only\nfree the first `s` out of `used_atom_cnt`-many and leak the rest. Make\nsure we free all items in `value`.\n\nIn the caller, `ref_array_clear()`, this means we need to be careful not\nto zero `used_atom_cnt` until after we've called `free_array_item()`. We\ncould move just a single line, but let's keep related things close\ntogether instead, by first handling `array`, then `used_atom`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n ref-filter.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 791f0648a6..1c1a2af880 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2105,7 +2105,9 @@ static void free_array_item(struct ref_array_item *item)\n {\n \tfree((char *)item->symref);\n \tif (item->value) {\n-\t\tfree((char *)item->value->s);\n+\t\tint i;\n+\t\tfor (i = 0; i < used_atom_cnt; i++)\n+\t\t\tfree((char *)item->value[i].s);\n \t\tfree(item->value);\n \t}\n \tfree(item);\n@@ -2116,14 +2118,16 @@ void ref_array_clear(struct ref_array *array)\n {\n \tint i;\n \n-\tfor (i = 0; i < used_atom_cnt; i++)\n-\t\tfree((char *)used_atom[i].name);\n-\tFREE_AND_NULL(used_atom);\n-\tused_atom_cnt = 0;\n \tfor (i = 0; i < array->nr; i++)\n \t\tfree_array_item(array->items[i]);\n \tFREE_AND_NULL(array->items);\n \tarray->nr = array->alloc = 0;\n+\n+\tfor (i = 0; i < used_atom_cnt; i++)\n+\t\tfree((char *)used_atom[i].name);\n+\tFREE_AND_NULL(used_atom);\n+\tused_atom_cnt = 0;\n+\n \tif (ref_to_worktree_map.worktrees) {\n \t\thashmap_free(&(ref_to_worktree_map.map), 1);\n \t\tfree_worktrees(ref_to_worktree_map.worktrees);\n-- \n2.22.0.428.g6d5b264208\n\n"},{"id":"378779","messageId":"xmqqr26xsjv7.fsf@gitster-ct.c.googlers.com","threadId":"51459","inReplyTo":"20190710183639.4123992-1-martin.agren@gmail.com","subject":"Re: [PATCH] ref-filter: fix memory leak in `free_array_item()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-10T20:41:16Z","receivedAt":"2019-07-10T20:41:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> We treat the `value` pointer as a pointer to a struct and free its `s`\n> field. But `value` is in fact an array of structs. As a result, we only\n> free the first `s` out of `used_atom_cnt`-many and leak the rest. Make\n> sure we free all items in `value`.\n\nThanks for spotting.  We do allocate an array of used_atom_cnt elements\nin populate_value() and we need to free them all.\n\n> In the caller, `ref_array_clear()`, this means we need to be careful not\n> to zero `used_atom_cnt` until after we've called `free_array_item()`. We\n> could move just a single line, but let's keep related things close\n> together instead, by first handling `array`, then `used_atom`.\n\nYup.  Looking good.\n\nThanks.\n\n>\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  ref-filter.c | 14 +++++++++-----\n>  1 file changed, 9 insertions(+), 5 deletions(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 791f0648a6..1c1a2af880 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2105,7 +2105,9 @@ static void free_array_item(struct ref_array_item *item)\n>  {\n>  \tfree((char *)item->symref);\n>  \tif (item->value) {\n> -\t\tfree((char *)item->value->s);\n> +\t\tint i;\n> +\t\tfor (i = 0; i < used_atom_cnt; i++)\n> +\t\t\tfree((char *)item->value[i].s);\n>  \t\tfree(item->value);\n>  \t}\n>  \tfree(item);\n> @@ -2116,14 +2118,16 @@ void ref_array_clear(struct ref_array *array)\n>  {\n>  \tint i;\n>  \n> -\tfor (i = 0; i < used_atom_cnt; i++)\n> -\t\tfree((char *)used_atom[i].name);\n> -\tFREE_AND_NULL(used_atom);\n> -\tused_atom_cnt = 0;\n>  \tfor (i = 0; i < array->nr; i++)\n>  \t\tfree_array_item(array->items[i]);\n>  \tFREE_AND_NULL(array->items);\n>  \tarray->nr = array->alloc = 0;\n> +\n> +\tfor (i = 0; i < used_atom_cnt; i++)\n> +\t\tfree((char *)used_atom[i].name);\n> +\tFREE_AND_NULL(used_atom);\n> +\tused_atom_cnt = 0;\n> +\n>  \tif (ref_to_worktree_map.worktrees) {\n>  \t\thashmap_free(&(ref_to_worktree_map.map), 1);\n>  \t\tfree_worktrees(ref_to_worktree_map.worktrees);\n"}]}