patchchdir-notify: UNLEAK registrated callback entries

> chdir_notify_register() allows registering functions to notify when
> chdir() is called.  There is no way to unsubscribe or shut this
> mechanism down, so these entries are present until the program ends.
>
> Valgrind reports allocations for these registrations as "possibly lost",
> probably because it doesn't see through list.h's offsetof tricks.

>  struct chdir_notify_entry {
>  	const char *name;
>  	chdir_notify_callback cb;
>  	void *data;
> -	struct list_head list;
>  };
> -static LIST_HEAD(chdir_notify_entries);
> +
> +#define VECTOR_TYPE(elemtype) struct { elemtype *v; size_t alloc, nr; }
> +#define VECTOR_FOR_EACH(x, p) for (p = (x)->v; p < (x)->v + (x)->nr; p++)
> +#define VECTOR_NEW_ENTRY(x, p) do {			\
> +	ALLOC_GROW_BY((x)->v, (x)->nr, 1, (x)->alloc);	\
> +	p = (x)->v + (x)->nr - 1;			\
> +} while (0)
> +
> +static VECTOR_TYPE(struct chdir_notify_entry) chdir_notify_entries;
>
>  void chdir_notify_register(const char *name,
>  			   chdir_notify_callback cb,
>  			   void *data)
>  {
> -	struct chdir_notify_entry *e = xmalloc(sizeof(*e));
> -	UNLEAK(e);
> +	struct chdir_notify_entry *e;
> +
> +	VECTOR_NEW_ENTRY(&chdir_notify_entries, e);
>  	e->name = name;
>  	e->cb = cb;
>  	e->data = data;
> -	list_add_tail(&e->list, &chdir_notify_entries);
>  }
>
>  static void reparent_cb(const char *name,
> @@ -52,7 +58,7 @@ void chdir_notify_reparent(const char *name, char **path)
>  int chdir_notify(const char *new_cwd)
>  {
>  	struct strbuf old_cwd = STRBUF_INIT;
> -	struct list_head *pos;
> +	struct chdir_notify_entry *e;
>
>  	if (strbuf_getcwd(&old_cwd) < 0)
>  		return -1;
> @@ -67,11 +73,8 @@ int chdir_notify(const char *new_cwd)
>  			 "setup: chdir from '%s' to '%s'",
>  			 old_cwd.buf, new_cwd);
>
> -	list_for_each(pos, &chdir_notify_entries) {
> -		struct chdir_notify_entry *e =
> -			list_entry(pos, struct chdir_notify_entry, list);
> +	VECTOR_FOR_EACH(&chdir_notify_entries, e)
>  		e->cb(e->name, old_cwd.buf, new_cwd, e->data);
> -	}
>
>  	strbuf_release(&old_cwd);
>  	return 0;
> --
> 2.29.2

>
> Am 14.11.20 um 22:40 schrieb René Scharfe:
> > chdir_notify_register() allows registering functions to notify when
> > chdir() is called.  There is no way to unsubscribe or shut this
> > mechanism down, so these entries are present until the program ends.
> >
> > Valgrind reports allocations for these registrations as "possibly lost",
> > probably because it doesn't see through list.h's offsetof tricks.
>
> Right, avoiding list.h like below lets Valgrind classify the memory as
> "still reachable", without UNLEAK.  Not sure if it's worth it, though.
>
> Note my somewhat concerning knee-jerk reaction to write some macros. ;)

> > Right, avoiding list.h like below lets Valgrind classify the memory as
> > "still reachable", without UNLEAK.  Not sure if it's worth it, though.
> >
> > Note my somewhat concerning knee-jerk reaction to write some macros. ;)
> 
> I've got a local patch that changes these to actually be freed as yet
> another route we could go.  It's mixed with a few (half-baked)
> revision-related memory cleanups that should be separated, and I'm
> unsure if there are other places that would need a finalize_globals()
> call too, but it's another approach to consider...

> diff --git a/builtin/fast-rebase.c b/builtin/fast-rebase.c
> index 19333ca42d..32cab36ad3 100644
> --- a/builtin/fast-rebase.c
> +++ b/builtin/fast-rebase.c
> @@ -173,6 +173,8 @@ int cmd_fast_rebase(int argc, const char **argv, const char *prefix)
>  		last_commit = create_commit(result.tree, commit, last_commit);
>  	}
>  	fprintf(stderr, "\nDone.\n");
> +	rev_info_free(&revs);
> +	memset(&revs, 0, sizeof(revs));

> diff --git a/cache.h b/cache.h
> index c0072d43b1..b9b543f75b 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -604,6 +604,9 @@ const char *setup_git_directory(void);
>  char *prefix_path(const char *prefix, int len, const char *path);
>  char *prefix_path_gently(const char *prefix, int len, int *remaining, const char *path);
>  
> +/* Release resources related to global config. */
> +void finalize_globals(void);

> +void chdir_deregister_all(void)
> +{
> +	struct list_head *pos, *tmp;
> +
> +	if (list_empty(&chdir_notify_entries))
> +		return;
> +
> +	list_for_each_safe(pos, tmp, &chdir_notify_entries) {
> +		struct chdir_notify_entry *e =
> +			list_entry(pos, struct chdir_notify_entry, list);
> +		free(e);
> +	}
> +	memset(&chdir_notify_entries, 0, sizeof(chdir_notify_entries));
> +}

> diff --git a/git.c b/git.c
> index af84f11e69..4b04988909 100644
> --- a/git.c
> +++ b/git.c
> @@ -358,6 +358,7 @@ static int handle_alias(int *argcp, const char ***argv)
>  			trace2_cmd_name("_run_shell_alias_");
>  
>  			ret = run_command(&child);
> +			finalize_globals();
>  			if (ret >= 0)   /* normal exit */
>  				exit(ret);

> +void rev_info_free(struct rev_info *revs)
> +{
> +	int i;
> +
> +	for (i = 0; i < revs->cmdline.nr; i++)
> +		free((char*)revs->cmdline.rev[i].name);
> +	free(revs->cmdline.rev);
> +}

>
> On Mon, Nov 16, 2020 at 08:49:23PM -0800, Elijah Newren wrote:
>
> > > Right, avoiding list.h like below lets Valgrind classify the memory as
> > > "still reachable", without UNLEAK.  Not sure if it's worth it, though.
> > >
> > > Note my somewhat concerning knee-jerk reaction to write some macros. ;)
> >
> > I've got a local patch that changes these to actually be freed as yet
> > another route we could go.  It's mixed with a few (half-baked)
> > revision-related memory cleanups that should be separated, and I'm
> > unsure if there are other places that would need a finalize_globals()
> > call too, but it's another approach to consider...
>
> Here are a few thoughts...
>
> > diff --git a/builtin/fast-rebase.c b/builtin/fast-rebase.c
> > index 19333ca42d..32cab36ad3 100644
> > --- a/builtin/fast-rebase.c
> > +++ b/builtin/fast-rebase.c
> > @@ -173,6 +173,8 @@ int cmd_fast_rebase(int argc, const char **argv, const char *prefix)
> >               last_commit = create_commit(result.tree, commit, last_commit);
> >       }
> >       fprintf(stderr, "\nDone.\n");
> > +     rev_info_free(&revs);
> > +     memset(&revs, 0, sizeof(revs));
>
> This one could easily be an UNLEAK(), since we're presumably exiting
> from cmd_fast_rebase() soon after. But I certainly don't mind a real
> cleanup.
>
> Probably rev_info_free() should leave the rev_info in a reasonable state
> (so callers don't have to re-zero it).
>
> > diff --git a/cache.h b/cache.h
> > index c0072d43b1..b9b543f75b 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -604,6 +604,9 @@ const char *setup_git_directory(void);
> >  char *prefix_path(const char *prefix, int len, const char *path);
> >  char *prefix_path_gently(const char *prefix, int len, int *remaining, const char *path);
> >
> > +/* Release resources related to global config. */
> > +void finalize_globals(void);
>
> Yuck. Remembering to put things in this, and remembering to call it in
> all of the right places seems ugly.
>
> If we want to go this route, then I think individual pieces of code
> could register their own atexit() handlers to clean up. But...
>
> > +void chdir_deregister_all(void)
> > +{
> > +     struct list_head *pos, *tmp;
> > +
> > +     if (list_empty(&chdir_notify_entries))
> > +             return;
> > +
> > +     list_for_each_safe(pos, tmp, &chdir_notify_entries) {
> > +             struct chdir_notify_entry *e =
> > +                     list_entry(pos, struct chdir_notify_entry, list);
> > +             free(e);
> > +     }
> > +     memset(&chdir_notify_entries, 0, sizeof(chdir_notify_entries));
> > +}
>
> I disagree that this is actually a leak. The memory is still referenced
> by the list, just like a ton of other global variables (e.g., quite a
> lot of config or environment variables that are loaded throughout a
> normal program).
>
> So my inclination is to leave referenced globals like this. If there are
> tools that are confused by our linked list structure, then we should
> adjust our tools (either using ASan, or turning off valgrind's "possibly
> lost" report).
>
> (My preference is to use ASan in general, because it's _so_ much
> faster. Running the whole suite under valgrind is pretty painful, and
> ideally we'd eventually hit a point where it was possible to run the
> whole suite with leak-checking enabled).

> > diff --git a/git.c b/git.c
> > index af84f11e69..4b04988909 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -358,6 +358,7 @@ static int handle_alias(int *argcp, const char ***argv)
> >                       trace2_cmd_name("_run_shell_alias_");
> >
> >                       ret = run_command(&child);
> > +                     finalize_globals();
> >                       if (ret >= 0)   /* normal exit */
> >                               exit(ret);
>
> I thought common-main.c might be a better place to put something like
> this, but any time we exit() it gets weird (of course, atexit() would
> take care of this).
>
> OTOH, if we call exit() then most things are still on the stack (or in
> globals), so we're not actually leaking those things. I think the root
> of this chdir-notify issue is not that we don't still have a handle to
> the memory, but that the linked list confuses valgrind.  But the same
> would be true if we were holding another linked list (or hashmap) in a
> stack variable, referenced via the heap, etc, and called exit().
>
> > +void rev_info_free(struct rev_info *revs)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < revs->cmdline.nr; i++)
> > +             free((char*)revs->cmdline.rev[i].name);
> > +     free(revs->cmdline.rev);
> > +}
>
> I suspect there's quite a bit more needed here (René already found a
> case where pathspecs needed to be freed). But I don't mind if we start
> small and add to it as leak-checking notices items.

> chdir_notify_register() allows registering functions to notify when
> chdir() is called.  There is no way to unsubscribe or shut this
> mechanism down, so these entries are present until the program ends.
> 
> Valgrind reports allocations for these registrations as "possibly lost",
> probably because it doesn't see through list.h's offsetof tricks.
> Annotate them using UNLEAK, which causes Valgrind to report them as
> "still reachable" instead.

> On Sat, Nov 14, 2020 at 10:40:01PM +0100, René Scharfe wrote:
>
>> chdir_notify_register() allows registering functions to notify when
>> chdir() is called.  There is no way to unsubscribe or shut this
>> mechanism down, so these entries are present until the program ends.
>>
>> Valgrind reports allocations for these registrations as "possibly lost",
>> probably because it doesn't see through list.h's offsetof tricks.
>> Annotate them using UNLEAK, which causes Valgrind to report them as
>> "still reachable" instead.
>
> I can't say I'm excited to see UNLEAK used here. It was really intended
> for items going out of scope that weren't worth cleaning up. But here
> we're papering over a failure in the memory checking tool for something
> that _is_ in scope.

> If we did want to paper over this case for valgrind, I think this is a
> better way to do so:
>
> diff --git a/chdir-notify.c b/chdir-notify.c
> index 5f7f2c2ac2..ddfe703b1a 100644
> --- a/chdir-notify.c
> +++ b/chdir-notify.c
> @@ -4,10 +4,10 @@
>  #include "strbuf.h"
>
>  struct chdir_notify_entry {
> +	struct list_head list;
>  	const char *name;
>  	chdir_notify_callback cb;
>  	void *data;
> -	struct list_head list;
>  };
>  static LIST_HEAD(chdir_notify_entries);

> I also wonder if valgrind _is_ aware of the distinction, and that's why
> these show up as only "possibly lost". And indeed, the faq[1] says:
>
>  - "possibly lost" means your program is leaking memory, unless you're
>    doing unusual things with pointers that could cause them to point
>    into the middle of an allocated block; see the user manual for some
>    possible causes. Use --show-possibly-lost=no if you don't want to see
>    these reports.
>
> and the user manual[2] has a more elaborate example that calls these
> "interior pointers". So I think that's exactly what is going on here.
>
> But then I'm not sure why we'd want this patch. List pointers (and now
> hashmap entries, which also contain a linked-list chain) are used in
> lots of data structures. Fixing this one case manually is not that
> interesting. If we're going to use valgrind, we probably need to accept
> that its "possibly lost" distinction is not useful for our code and turn
> it off.