From: Andrzej Hunt <andrzej@ahunt.org>
Date: Sun, 14 Mar 2021 18:07:18 GMT
Subject: Re: [PATCH 1/7] symbolic-ref: don't leak shortened refname in check_symref()
Message-ID: <8d413cdb-e264-8381-7254-522c3453f188@ahunt.org>
In-Reply-To: <YEZ0jLppB9wOg/af@coredump.intra.peff.net>


On 08/03/2021 20:01, Jeff King wrote:
> On Mon, Mar 08, 2021 at 06:36:14PM +0000, Andrzej Hunt via GitGitGadget wrote:
> 
>> From: Andrzej Hunt <ajrhunt@google.com>
>>
>> This leak has existed since:
>> 9ab55daa55 (git symbolic-ref --delete $symref, 2012-10-21)
>>
>> This leak was found when running t0001 with LSAN, see also LSAN output
>> below:
>>
>> Direct leak of 19 byte(s) in 1 object(s) allocated from:
>>      #0 0x486514 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
>>      #1 0x9ab048 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
>>      #2 0x8b452f in refs_shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c
>>      #3 0x8b47e8 in shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c:1287:9
>>      #4 0x679fce in check_symref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:28:14
>>      #5 0x679ad8 in cmd_symbolic_ref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:70:9
>>      #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
>>      #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
>>      #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
>>      #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
>>      #10 0x69cc6e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
>>      #11 0x7f98388a4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
> 
> As a general template for fixing leaks, this information seems pretty
> good. You might want to give a brief reason for why it's a leak (like
> you do already in the second patch). Here it just would be something
> like:
> 
>    shorten_unambiguous_ref() returns an allocated string. We have to
>    track it separately from the const refname.
> 
> Or whatever. It doesn't need to be a novel, but just give an overview of
> what's going that makes the diff obvious.

Good point - I've copied this one verbatim - but it's also a good thing 
to remember if/when I fix further leaks!

> 
> There's also an idiom in Git's codebase when a const pointer may alias
> unowned storage, or a buffer that needs to be freed. Something like:
> 
>    if (print) {
>            char *to_free = NULL;
> 	  if (shorten)
> 	          refname = to_free = shorten_unambiguous_ref(refname, 0);
> 	  puts(refname);
> 	  free(to_free);
>    }
> 
> That avoids duplicating the part of the code that handles the variable.
> In this case it is only a single line, but IMHO it's still easier to
> read, as it makes clear that we call puts() in either case.

That's a nice pattern, and will probably be useful for future leak fixes 
too - I've made this change too!