Re: [PATCH 02/12] remote.c: drop "remote" pointer from "struct branch"

On Tue, May 05, 2015 at 03:31:05PM -0400, Jeff King wrote:

One thing I did notice while looking at this is that it seems like we
may leak if you call branch_get multiple times. The make_branch()
function may sometimes return a brand-new branch and sometimes return a
cached version from the "branches" array. In the latter case, we
continue to update the "remote" pointer (which is wasteful but at least
does not leak because the remotes themselves are part of a cached list).
But then we will repeatedly re-allocate the ret->merge array. We
probably should make sure it is NULL before trying to fill it in.

I'll see if I can insert a cleanup patch in this part of the series.

Here's what I came up with. I'm sending it separately in case you have
any early comments, but it will be part of the next re-roll (just before
the existing patch 2 we're discussing here).

-- >8 --
Subject: remote.c: refactor setup of branch->merge list

When we call branch_get() to lookup or create a "struct
branch", we make sure the "merge" field is filled in so that
callers can access it. But the conditions under which we do
so are a little confusing, and can lead to two funny
situations:

  1. If there's no branch.*.remote config, we cannot provide
     branch->merge (because it is really just an application
     of branch.*.merge to our remote's refspecs). But
     branch->merge_nr may be non-zero, leading callers to be
     believe they can access branch->merge (e.g., in
     branch_merge_matches and elsewhere).

     It doesn't look like this can cause a segfault in
     practice, as most code paths dealing with merge config
     will bail early if there is no remote defined. But it's
     a bit of a dangerous construct.

     We can fix this by setting merge_nr to "0" explicitly
     when we realize that we have no merge config. Note that
     merge_nr also counts the "merge_name" fields (which we
     _do_ have; that's how merge_nr got incremented), so we
     will "lose" access to them, in the sense that we forget
     how many we had. But no callers actually care; we use
     merge_name only while iteratively reading the config,
     and then convert it to the final "merge" form the first
     time somebody calls branch_get().

  2. We set up the "merge" field every time branch_get is
     called, even if it has already been done. This leaks
     memory.

     It's not a big deal in practice, since most code paths
     will access only one branch, or perhaps each branch
     only one time. But if you want to be pathological, you
     can leak arbitrary memory with:

       yes @{upstream} | head -1000 | git rev-list --stdin

     We can fix this by skipping setup when branch->merge is
     already non-NULL.

In addition to those two fixes, this patch pushes the "do we
need to setup merge?" logic down into set_merge, where it is
a bit easier to follow.

Signed-off-by: Jeff King <redacted>
---
 remote.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/remote.c b/remote.c
index bec8b31..ac17e66 100644
--- a/remote.c
+++ b/remote.c

@@ -1636,6 +1636,19 @@ static void set_merge(struct branch *ret)
 	unsigned char sha1[20];
 	int i;
 
+	if (!ret)
+		return; /* no branch */
+	if (ret->merge)
+		return; /* already run */
+	if (!ret->remote_name || !ret->merge_nr) {
+		/*
+		 * no merge config; let's make sure we don't confuse callers
+		 * with a non-zero merge_nr but a NULL merge
+		 */
+		ret->merge_nr = 0;
+		return;
+	}
+
 	ret->merge = xcalloc(ret->merge_nr, sizeof(*ret->merge));
 	for (i = 0; i < ret->merge_nr; i++) {
 		ret->merge[i] = xcalloc(1, sizeof(**ret->merge));

@@ -1660,11 +1673,9 @@ struct branch *branch_get(const char *name)
 		ret = current_branch;
 	else
 		ret = make_branch(name, 0);
-	if (ret && ret->remote_name) {
+	if (ret && ret->remote_name)
 		ret->remote = remote_get(ret->remote_name);
-		if (ret->merge_nr)
-			set_merge(ret);
-	}
+	set_merge(ret);
 	return ret;
 }
 

-- 
2.4.0.488.gf55b16a