[8/9] ref-filter: fix leak when formatting %(push:remoteref)

Commit Message

Jeff King Sept. 9, 2024, 11:19 p.m. UTC

When we expand the %(upstream) or %(push) placeholders, we rely on
remote.c's remote_ref_for_branch() to fill in the ":refname" argument.
But that function has confusing memory ownership semantics: it may or
may not return an allocated string, depending on whether we are in
"upstream" mode or "push" mode. The caller in ref-filter.c always
duplicates the result, meaning that we leak the original in the case of
%(push:refname).

To solve this, let's make the return value from remote_ref_for_branch()
consistent, by always returning an allocated pointer. Note that the
switch to returning a non-const pointer has a ripple effect inside the
function, too. We were storing the "dst" result as a const pointer, too,
even though it is always allocated! It is the return value from
apply_refspecs(), which is always a non-const allocated string.

And then on the caller side in ref-filter.c (and this is the only caller
at all), we just need to avoid the extra duplication when the return
value is non-NULL.

This clears up one case that LSan finds in t6300, but there are more.

Signed-off-by: Jeff King <[e-mail]>
---
 ref-filter.c | 2 +-
 remote.c     | 8 ++++----
 remote.h     | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

Comments

Patrick Steinhardt Sept. 10, 2024, 6:09 a.m. UTC | #1

On Mon, Sep 09, 2024 at 07:19:51PM -0400, Jeff King wrote:
> When we expand the %(upstream) or %(push) placeholders, we rely on
> remote.c's remote_ref_for_branch() to fill in the ":refname" argument.
> But that function has confusing memory ownership semantics: it may or
> may not return an allocated string, depending on whether we are in
> "upstream" mode or "push" mode. The caller in ref-filter.c always
> duplicates the result, meaning that we leak the original in the case of
> %(push:refname).

Ah, I remember this issue, I think I also have it pending somewhere.
Anyway, I'm happy if I can drop one more patch.

The change looks sensible to me.

Patrick

Patch

diff --git a/ref-filter.c b/ref-filter.c
index 370cc5b44a..0f51095bbd 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2237,7 +2237,7 @@  static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 		const char *merge;
 
 		merge = remote_ref_for_branch(branch, atom->u.remote_ref.push);
-		*s = xstrdup(merge ? merge : "");
+		*s = merge ? merge : xstrdup("");
 	} else
 		BUG("unhandled RR_* enum");
 }
diff --git a/remote.c b/remote.c
index 8f3dee1318..539e5ceae3 100644
--- a/remote.c
+++ b/remote.c
@@ -632,19 +632,19 @@  const char *pushremote_for_branch(struct branch *branch, int *explicit)
 static struct remote *remotes_remote_get(struct remote_state *remote_state,
 					 const char *name);
 
-const char *remote_ref_for_branch(struct branch *branch, int for_push)
+char *remote_ref_for_branch(struct branch *branch, int for_push)
 {
 	read_config(the_repository, 0);
 	die_on_missing_branch(the_repository, branch);
 
 	if (branch) {
 		if (!for_push) {
 			if (branch->merge_nr) {
-				return branch->merge_name[0];
+				return xstrdup(branch->merge_name[0]);
 			}
 		} else {
-			const char *dst,
-				*remote_name = remotes_pushremote_for_branch(
+			char *dst;
+			const char *remote_name = remotes_pushremote_for_branch(
 					the_repository->remote_state, branch,
 					NULL);
 			struct remote *remote = remotes_remote_get(
diff --git a/remote.h b/remote.h
index b901b56746..a58713f20a 100644
--- a/remote.h
+++ b/remote.h
@@ -329,7 +329,7 @@  struct branch {
 struct branch *branch_get(const char *name);
 const char *remote_for_branch(struct branch *branch, int *explicit);
 const char *pushremote_for_branch(struct branch *branch, int *explicit);
-const char *remote_ref_for_branch(struct branch *branch, int for_push);
+char *remote_ref_for_branch(struct branch *branch, int for_push);
 
 /* returns true if the given branch has merge configuration given. */
 int branch_has_merge_config(struct branch *branch);