Re: [PATCH 1/2] commit-graph: clean up leaked memory during write

From: Martin Ågren <martin.agren@gmail.com>
Date: Oct 2, 2018, 15:40 UTC
Message-ID: <CAN0heSomOen+mWg1CuPGUCJFgmDQUjGLtAdxpWLwrStRU4Me8g@mail.gmail.com>
In-Reply-To: <6906c25415eddf79cc3f71f905a77b140f2f66f0.1538492321.git.gitgitgadget@gmail.com>

On Tue, 2 Oct 2018 at 17:01, Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 11 quoted lines

> diff --git a/commit-graph.c b/commit-graph.c
> index 2a24eb8b5a..7226bd6b58 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -698,6 +698,8 @@ void write_commit_graph_reachable(const char *obj_dir, int append,
>         string_list_init(&list, 1);
>         for_each_ref(add_ref_to_list, &list);
>         write_commit_graph(obj_dir, NULL, &list, append, report_progress);
> +
> +       string_list_clear(&list, 0);
>  }

Nit: The blank line adds some asymmetry, IMVHO.

Show 11 quoted lines

>  void write_commit_graph(const char *obj_dir,
> @@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,
>         compute_generation_numbers(&commits, report_progress);
>
>         graph_name = get_commit_graph_filename(obj_dir);
> -       if (safe_create_leading_directories(graph_name))
> +       if (safe_create_leading_directories(graph_name)) {
> +               UNLEAK(graph_name);
>                 die_errno(_("unable to create leading directories of %s"),
>                           graph_name);
> +       }

Do you really need this hunk? In my testing with LeakSanitizer and valgrind, I don't need this hunk to be leak-free. Generally speaking, it seems impossible to UNLEAK when dying, since we don't know what we have allocated higher up in the call-stack.

Without this hunk, this patch can have my

Reviewed-by: Martin Ågren <martin.agren@gmail.com>

as I've verified the leaks before and after. With this hunk, I am puzzled and feel uneasy, both about having to UNLEAK before dying and about having to UNLEAK outside of builtin/.

Show 5 quoted lines

> +       free(graph_name);
> +       free(commits.list);
>         free(oids.list);
>         oids.alloc = 0;
>         oids.nr = 0;

Both `commits` and `oids` are on the stack here, so cleaning up one more than the other is a bit asymmetrical. Also, if we try to zero the counts -- which seems unnecessary to me, but which is not new with this patch -- we should perhaps use `FREE_AND_NULL` too. But personally, I would just use `free` and leave `nr` and `alloc` at whatever values they happen to have.

Martin