[4/7] worktree: fix leak in dwim_branch()

Commit Message

Andrzej Hunt March 8, 2021, 6:36 p.m. UTC

From: Andrzej Hunt <[e-mail]>

Make sure that we release the temporary strbuf during dwim_branch() for
all codepaths (and not just for the early return).

This leak appears to have been introduced in:
  f60a7b763f (worktree: teach "add" to check out existing branches, 2018-04-24)

Note that UNLEAK(branchname) is still needed: the returned result is
used in add(), and is stored in a pointer which is used to point at one
of:
  - a string literal ("HEAD")
  - member of argv (whatever the user specified in their invocation)
  - or our newly allocated string returned from dwim_branch()
Fixing the branchname leak isn't impossible, but does not seem
worthwhile given that add() is called directly from cmd_main(), and
cmd_main() returns immediately thereafter - UNLEAK is good enough.

This leak was found when running t0001 with LSAN, see also LSAN output
below:

Direct leak of 60 byte(s) in 1 object(s) allocated from:
    #0 0x49a859 in realloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0x9ab076 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
    #2 0x939fcd in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
    #3 0x93af53 in strbuf_splice /home/ahunt/oss-fuzz/git/strbuf.c:239:3
    #4 0x83559a in strbuf_check_branch_ref /home/ahunt/oss-fuzz/git/object-name.c:1593:2
    #5 0x6988b9 in dwim_branch /home/ahunt/oss-fuzz/git/builtin/worktree.c:454:20
    #6 0x695f8f in add /home/ahunt/oss-fuzz/git/builtin/worktree.c:525:19
    #7 0x694a04 in cmd_worktree /home/ahunt/oss-fuzz/git/builtin/worktree.c:1036:10
    #8 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #9 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #10 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #11 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #12 0x69caee in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #13 0x7f7b7dd10349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Signed-off-by: Andrzej Hunt <[e-mail]>
---
 builtin/worktree.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Jeff King March 8, 2021, 7:16 p.m. UTC | #1

On Mon, Mar 08, 2021 at 06:36:17PM +0000, Andrzej Hunt via GitGitGadget wrote:

> Make sure that we release the temporary strbuf during dwim_branch() for
> all codepaths (and not just for the early return).

Makes sense. Two style nits:

>  static const char *dwim_branch(const char *path, const char **new_branch)
>  {
> -	int n;
> +	int n, branch_exists;

If two variables aren't strictly related, but just happen to share the
same type, IMHO it's better to declare them separately. (There are
numerous spots in Git's code that don't follow this rule, but I think we
should be moving in that direction).

> -	if (!strbuf_check_branch_ref(&ref, branchname) &&
> -	    ref_exists(ref.buf)) {
> -		strbuf_release(&ref);
> +
> +	branch_exists = (!strbuf_check_branch_ref(&ref, branchname) &&
> +			 ref_exists(ref.buf));

We'd usually omit the extra parentheses here. I.e.,:

  branch_exists = !strbuf_check_branch_ref(&ref, branchname) &&
                  ref_exists(ref.buf);

-Peff

Andrzej Hunt March 14, 2021, 5:56 p.m. UTC | #2

On 08/03/2021 20:16, Jeff King wrote:
> On Mon, Mar 08, 2021 at 06:36:17PM +0000, Andrzej Hunt via GitGitGadget wrote:
> 
>> Make sure that we release the temporary strbuf during dwim_branch() for
>> all codepaths (and not just for the early return).
> 
> Makes sense. Two style nits:

Thank, I'll fix both.
> 
>> -	if (!strbuf_check_branch_ref(&ref, branchname) &&
>> -	    ref_exists(ref.buf)) {
>> -		strbuf_release(&ref);
>> +
>> +	branch_exists = (!strbuf_check_branch_ref(&ref, branchname) &&
>> +			 ref_exists(ref.buf));
> 
> We'd usually omit the extra parentheses here. I.e.,:
> 
>    branch_exists = !strbuf_check_branch_ref(&ref, branchname) &&
>                    ref_exists(ref.buf);

I've made this change - but I have a question about the formatting:
- In the few examples that I could find, the second line is aligned 
using spaces (i.e. same number of tabs as the previous line, followed by 
spaces to align correctly). However that appears to violate the 
indent-with-non-tab style check - should I switch to 100% tabs instead?

That check has been around since:

e2f6331a14 (.gitattributes: CR at the end of the line is an error, 
2009-06-19)

So maybe it's being intentionally ignored in the cases that I've seen (I 
only noticed it for my series because of the automated checks on Github) 
- but I thought I should ask to sure.

Patch

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 1cd5c2016e3f..44d3f058d065 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -445,17 +445,17 @@  static void print_preparing_worktree_line(int detach,
 
 static const char *dwim_branch(const char *path, const char **new_branch)
 {
-	int n;
+	int n, branch_exists;
 	const char *s = worktree_basename(path, &n);
 	const char *branchname = xstrndup(s, n);
 	struct strbuf ref = STRBUF_INIT;
-
 	UNLEAK(branchname);
-	if (!strbuf_check_branch_ref(&ref, branchname) &&
-	    ref_exists(ref.buf)) {
-		strbuf_release(&ref);
+
+	branch_exists = (!strbuf_check_branch_ref(&ref, branchname) &&
+			 ref_exists(ref.buf));
+	strbuf_release(&ref);
+	if (branch_exists)
 		return branchname;
-	}
 
 	*new_branch = branchname;
 	if (guess_remote) {