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

From: Jeff King <peff@peff.net>
Date: Nov 17, 2020, 00:24 UTC
Message-ID: <20201117002435.GA13516@coredump.intra.peff.net>
In-Reply-To: <8a9cf9ba-f615-3ff5-8d35-c24c22d5d859@web.de>

On Sat, Nov 14, 2020 at 10:40:01PM +0100, René Scharfe wrote:

Show 8 quoted lines

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

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

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.

-Peff

[1] https://valgrind.org/docs/manual/faq.html#faq.deflost

[2] https://valgrind.org/docs/manual/mc-manual.html#mc-manual.leaks