| Message ID | ff0f7c167fa5b5da2d843fa03edb70907e6af763.1615228580.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix all leaks in t0001 | expand |
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. > diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c > index 80237f0df10f..8cf52599693a 100644 > --- a/builtin/symbolic-ref.c > +++ b/builtin/symbolic-ref.c > @@ -24,9 +24,15 @@ static int check_symref(const char *HEAD, int quiet, int shorten, int print) > return 1; > } > if (print) { > - if (shorten) > - refname = shorten_unambiguous_ref(refname, 0); > - puts(refname); > + if (shorten) { > + const char *shortened_refname; > + > + shortened_refname = shorten_unambiguous_ref(refname, 0); > + puts(shortened_refname); > + free((void *)shortened_refname); > + } else { > + puts(refname); > + } If a variable is meant to take ownership of memory, our usual convention is to not declare it as "const". And then you don't need to cast to pass it to free(). 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. -Peff
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!
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c index 80237f0df10f..8cf52599693a 100644 --- a/builtin/symbolic-ref.c +++ b/builtin/symbolic-ref.c @@ -24,9 +24,15 @@ static int check_symref(const char *HEAD, int quiet, int shorten, int print) return 1; } if (print) { - if (shorten) - refname = shorten_unambiguous_ref(refname, 0); - puts(refname); + if (shorten) { + const char *shortened_refname; + + shortened_refname = shorten_unambiguous_ref(refname, 0); + puts(shortened_refname); + free((void *)shortened_refname); + } else { + puts(refname); + } } return 0; }