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

[Stefan Beller <sbeller@google.com>]

On Tue, Oct 2, 2018 at 8:40 AM Martin Ågren <martin.agren@gmail.com> wrote:
>
> On Tue, 2 Oct 2018 at 17:01, Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > 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.

I think these blank lines are super common, as in:

    {
      declarations;

      multiple;
      lines(of);
      code;

      cleanup;
      and_frees;
    }

(c.f. display_table in column.c, which I admit to have
cherry-picked as an example).

While in nit territory, I would rather move the string list init
into the first block:

  {
    struct string_list list = STRING_LIST_INIT_DUP;

    for_each_ref(add_ref_to_list, &list);
    write_commit_graph(obj_dir, NULL, &list, append);

    string_list_clear(&list, 0);
  }




>
> >  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?

graph_name is produced via xstrfmt in get_commit_graph_filename,
so it needs to be free'd in any return/exit path.

> 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.

I do not understand; I thought UNLEAK was specifically for the purpose of
die() calls without imposing extra overhead; rereading 0e5bba53af
(add UNLEAK annotation for reducing leak false positives, 2017-09-08)
doesn't provide an example for prematurely die()ing, only for regular
program exit.

> 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/.

I am not uneasy about an UNLEAK before dying, but about dying outside
builtin/ in general (but having a die call accompanied by UNLEAK seems
to be the right thing). Can you explain the worries you have regarding the
allocations on the call stack, as xstrfmt is allocating on the heap and we
only UNLEAK the pointer to that?

Stefan