Re: [PATCH] chdir-notify: UNLEAK registrated callback entries

["René Scharfe" <l.s.r@web.de>]

Am 17.11.20 um 01:24 schrieb Jeff King:
> 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.

Right, though I'd consider every use of UNLEAK as "papering over".  And
not being able to distinguish with certainty between pointer trickery
and accidents is more of a missing (or impossible) feature than a
failure in my book.

> I guess I'm not too surprised that valgrind has trouble with list.h. We
> have pointers into a heap-allocated block, but not the start of it.
> Curiously, ASan/LSan get this case right. So my first instinct is: use
> those tools, they're better. :)

Does Leak Sanitizer get it right or is it just as unsure, but reports
its findings more cautiously?

> 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);

A trick to hide the trick -- I like it.

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

That would probably disregard real issues as well -- but it's hard to
know how many without finding and classifying them.

Lists (and hashmaps) have cleanup methods that we could call, as Elijah
noted.  We can skip that in main() functions for production builds, but
cutting out the noise when SUPPRESS_ANNOTATED_LEAKS is set would surely
make finding real leaks easier.

René