Re: [PATCH 10/13] remote.c: make match_refs() copy src ref before assigning to peer_ref

Jay Soffian [off-list ref] writes:

This issue existed before, but the other callers of match_heads() only
call it once before the process terminates, and don't bother to free
what it returns. So those callers didn't notice. This topic introduces
the first caller that calls match_heads() repeatedly, so it frees the
returned ref list after it has copied out what it needs.

The change is independent and can be moved anywhere in the series, or
even introduced as a patch independent of the series itself.

Thanks for explanation.

If I am reading the above correctly, a sequence that does not have 10/13
before the patch that makes match_heads() called repeatedly would break
bisection, so it cannot be "can be moved anywhere".

And it sounds like keeping this as a separate patch would make more
sense.  Your proposed commit log message for the patch would contain the
first paragraph I quoted above, I'd think.

In some instances, match_refs() sets the peer_ref field of refs in the
dst list such that it points to a ref in the src list. Were a caller to
free both the src and dst lists, this would cause a double-free as
free_refs() frees the peer_ref.

A double-free would also occur freeing just the dst list under the
following configuration:

  push = refs/heads/master:refs/heads/backup
  push = refs/heads/master:refs/heads/master

Since such a configuration would cause two refs in the dst list to have
the same peer_ref.

This patch corrects the problem by copying the ref when it has been
plucked from the src list.

This issue has gone undetected as the existing callers of match_heads()
call it only once and then terminate, w/o ever bothering to free the src
or dst lists.

Signed-off-by: Jay Soffian <redacted>
---
 remote.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index d7079c6..98575be 100644
--- a/remote.c
+++ b/remote.c

@@ -927,6 +927,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 			  struct refspec *rs)
 {
 	struct ref *matched_src, *matched_dst;
+	int copy_src;
 
 	const char *dst_value = rs->dst;
 	char *dst_guess;

@@ -937,6 +938,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 	matched_src = matched_dst = NULL;
 	switch (count_refspec_match(rs->src, src, &matched_src)) {
 	case 1:
+		copy_src = 1;
 		break;
 	case 0:
 		/* The source could be in the get_sha1() format

@@ -946,6 +948,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		matched_src = try_explicit_object_name(rs->src);
 		if (!matched_src)
 			return error("src refspec %s does not match any.", rs->src);
+		copy_src = 0;
 		break;
 	default:
 		return error("src refspec %s matches more than one.", rs->src);

@@ -991,7 +994,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		return error("dst ref %s receives from more than one src.",
 		      matched_dst->name);
 	else {
-		matched_dst->peer_ref = matched_src;
+		matched_dst->peer_ref = copy_src ? copy_ref(matched_src) : matched_src;
 		matched_dst->force = rs->force;
 	}
 	return 0;

@@ -1099,7 +1102,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 			dst_peer = make_linked_ref(dst_name, dst_tail);
 			hashcpy(dst_peer->new_sha1, src->new_sha1);
 		}
-		dst_peer->peer_ref = src;
+		dst_peer->peer_ref = copy_ref(src);
 		dst_peer->force = pat->force;
 	free_name:
 		free(dst_name);

-- 
1.6.2.rc1.291.g83eb