Re: UNLEAK(), leak checking in the default tests etc.

[Andrzej Hunt <andrzej@ahunt.org>]

On 09/06/2021 16:38, Ævar Arnfjörð Bjarmason wrote:
> 
> [In-Reply-To
> <a74bbcae7363df03bf8e93167d9274d16dc807f3.1615747662.git.gitgitgadget@gmail.com>,
> but intentionally breaking threading for a new topic]
> 
> On Sun, Mar 14 2021, Andrzej Hunt via GitGitGadget wrote:
> 
>> Most of these pointers can safely be freed when cmd_clone() completes,
>> therefore we make sure to free them. The one exception is that we
>> have to UNLEAK(repo) because it can point either to argv[0], or a
>> malloc'd string returned by absolute_pathdup().
> 
> I ran into this when manually checking with valgrind and discovered that
> you need SANITIZERS for -DSUPPRESS_ANNOTATED_LEAKS to squash it.
> 
> I wonder if that shouldn't be in DEVOPTS (or even a default under
> DEVELOPER=1). I.e. you don't need any other special compile flags, just
> a compiled git that you then run under valgrind to spot this.

I'm not familiar with git's development conventions/philosophy, but my 
2c is that it's better not to enable it by default in order to minimise 
divergence from the code that users are running. OTOH it's not a major 
difference in behaviour so perhaps that's not a concern here.

More significantly: I get the impression it's easier to do leak checking 
using LSAN, which requires recompiling git anyway - at which point you 
get the flag for free - so how often will people actually perform leak 
checking with Valgrind in the first place?

> 
>>   builtin/clone.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index 51e844a2de0a..952fe3d8fc88 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -964,10 +964,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>   {
>>   	int is_bundle = 0, is_local;
>>   	const char *repo_name, *repo, *work_tree, *git_dir;
>> -	char *path, *dir, *display_repo = NULL;
>> +	char *path = NULL, *dir, *display_repo = NULL;
>>   	int dest_exists, real_dest_exists = 0;
>>   	const struct ref *refs, *remote_head;
>> -	const struct ref *remote_head_points_at;
>> +	struct ref *remote_head_points_at = NULL;
>>   	const struct ref *our_head_points_at;
>>   	struct ref *mapped_refs;
>>   	const struct ref *ref;
>> @@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>   	repo_name = argv[0];
>>   
>>   	path = get_repo_path(repo_name, &is_bundle);
>> -	if (path)
>> +	if (path) {
>> +		FREE_AND_NULL(path);
>>   		repo = absolute_pathdup(repo_name);
>> -	else if (strchr(repo_name, ':')) {
>> +	} else if (strchr(repo_name, ':')) {
>>   		repo = repo_name;
>>   		display_repo = transport_anonymize_url(repo);
>>   	} else
> 
> In this case it seems better to just have a :
> 
>      int repo_heap = 0;
> 
>      Then set "repo_heap = 1" in that absolute_pathdup(repo_name) branch,
>      and...
> 
>> @@ -1393,6 +1394,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>   	strbuf_release(&reflog_msg);
>>   	strbuf_release(&branch_top);
>>   	strbuf_release(&key);
>> +	free_refs(mapped_refs);
>> +	free_refs(remote_head_points_at);
>> +	free(dir);
>> +	free(path);
>> +	UNLEAK(repo);
> 
> Here do:
> 
>      if (repo_heap)
>          free(repo);
> 

Although this is possible, I don't think it's worth it: if UNLEAK 
already exists, we might as well use it here to make the code simpler. 
And UNLEAK is unlikely to go away anytime soon because... (continued below)

> But maybe there's some other out of the box way to make leak checking
> Just Work without special flags in this case. I'm just noting this one
> because it ended up being the only one that leaked unless I compiled
> with -DSUPPRESS_ANNOTATED_LEAKS. I was fixing some leaks in the bundle
> code.

There are trickier examples where a cmd_* function has a complex struct 
on the stack, and correctly clearing all allocated memory pointed to by 
its members (or in turn further children with potentially multiple 
levels of indirection) is a lot of work - and that work doesn't actually 
benefit the user in any way. In other words, we either need to be able 
to use UNLEAK to suppress certain classes of uninteresting memory leaks 
- which allows us to focus on the interesting/real leaks - or someone 
has to spend a lot of time doing cleanup by hand (and/or someone has to 
implement a bunch of new cleanup functions)).

In your example above, the UNLEAK can be avoided at the cost of one 
additional tracking variable - but in many other cases avoiding an 
UNLEAK is much more expensive. It's certainly valid to debate the merits 
of the UNLEAK here, but that won't remove the need for UNLEAK's 
existence in general.

(The most common example that I remember is where cmd_* has a rev_info, 
and AFAICT there's no one-liner to clean that up. Using UNLEAK is 
honestly the best approach there. I don't think I've actually submitted 
any patches doing this, but I have a few in my local backlog.)
> Anyway, getting to the "default tests" point. I fixed a memory leak, and
> wanted to it tested that the specific command doesn't leak in git's
> default tests.
> 
> Do we have such a thing, if not why not?
> 
> The closest I got to getting this was:
> 
>      GIT_VALGRIND_MODE=memcheck GIT_VALGRIND_OPTIONS="--leak-check=full --errors-for-leak-kinds=definite --error-exitcode=123" <SOME TEST> --valgrind

It's easy to perform leak-checking runs *if* you're OK recompiling with 
LSAN, instead of using valgrind. My usual recipe for running against a 
range of tests is something like:

   make SANITIZE=address,leak 
ASAN_OPTIONS="detect_leaks=1:abort_on_error=1" CFLAGS="-Og -g" 
T="\$(wildcard t00[0-9][0-9]-*.sh)" test

Additionally: I usually specify CC=clang, although gcc+LSAN has mostly 
been stable enough in my experience so you might be able to skip that.
(I've found ASAN+LSAN to be more stable than LSAN by itself, which is 
why I specify address+leak, but adding ASAN in turn requires overriding 
ASAN_OPTIONS to reenable leak checking.)

I don't know whether or not Valgrind is more/less effective at finding 
leaks, so being able to run the test suite under valgrind would be nice 
for comparison purposes though.

ATB,

   Andrzej