[PATCH 0/4] memory leaks in remote.c

[PATCH 0/4] memory leaks in remote.c

[Jeff King]

This fixes some memory leaks in remote.c. Not urgent, as they are quite
old, but they are newly triggered in the test suite by Harald's
hn/status-compare-with-push topic. So I think we'd want to build that
topic on top of these.

The first two are just preparatory cleanups. Patch 3 fixes the leak that
Harald's series triggers (and adds its own test, of course). Patch 4 is
a hypothetical leak that I don't think can be triggered in practice (so
it's more of a cleanup).

  [1/4]: remote: return non-const pointer from error_buf()
  [2/4]: remote: drop const return of tracking_for_push_dest()
  [3/4]: remote: fix leak in branch_get_push_1() with invalid "simple" config
  [4/4]: remote: always allocate branch.push_tracking_ref

 remote.c                | 24 ++++++++++++++----------
 remote.h                |  2 +-
 t/for-each-ref-tests.sh |  9 +++++++++
 3 files changed, 24 insertions(+), 11 deletions(-)

-Peff

[PATCH 1/4] remote: return non-const pointer from error_buf()

[Jeff King]

We have an error_buf() helper that functions a bit like our error()
helper, but returns NULL instead of -1. Its return type is "const char
*", but this is overly restrictive. If we use the helper in a function
that returns non-const "char *", the compiler will complain about
the implicit cast from const to non-const.

Meanwhile, the const in the helper is doing nothing useful, as it only
ever returns NULL. Let's drop the const, which will let us use it in
both types of function.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index b756ff6f15..3dc100be83 100644
--- a/remote.c
+++ b/remote.c
@@ -1831,7 +1831,7 @@ int branch_merge_matches(struct branch *branch,
 }
 
 __attribute__((format (printf,2,3)))
-static const char *error_buf(struct strbuf *err, const char *fmt, ...)
+static char *error_buf(struct strbuf *err, const char *fmt, ...)
 {
 	if (err) {
 		va_list ap;
-- 
2.53.0.rc0.338.g08aa8a9473

Re: [PATCH 1/4] remote: return non-const pointer from error_buf()

[Patrick Steinhardt]

On Mon, Jan 19, 2026 at 12:19:45AM -0500, Jeff King wrote:
> We have an error_buf() helper that functions a bit like our error()
> helper, but returns NULL instead of -1. Its return type is "const char
> *", but this is overly restrictive. If we use the helper in a function
> that returns non-const "char *", the compiler will complain about
> the implicit cast from const to non-const.
> 
> Meanwhile, the const in the helper is doing nothing useful, as it only
> ever returns NULL. Let's drop the const, which will let us use it in
> both types of function.

This function signature is indeed quite misleading, and I'd argue that
it continues to be so even after the change. I guess the intent is to
make it a bit easier to print an error in functions that return a
string.

I'm not really a huge fan of this, but it's not a fault of this patch
series, so let's read on.

Patrick

Re: [PATCH 1/4] remote: return non-const pointer from error_buf()

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> This function signature is indeed quite misleading, and I'd argue that
> it continues to be so even after the change. I guess the intent is to
> make it a bit easier to print an error in functions that return a
> string.
>
> I'm not really a huge fan of this, but it's not a fault of this patch
> series, so let's read on.

I concur.  "If they do not return any useful value, they should be
void" was my first reaction, but presumably just like "return
error("message");" is a handy way to give message while signalling
an error to the caller, these are used to return NULL that signals
an error?  I do not offhand think of a good longer-term direction to
improve this one.

Re: [PATCH 1/4] remote: return non-const pointer from error_buf()

[Jeff King]

On Mon, Jan 19, 2026 at 04:28:42PM -0800, Junio C Hamano wrote:

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > This function signature is indeed quite misleading, and I'd argue that
> > it continues to be so even after the change. I guess the intent is to
> > make it a bit easier to print an error in functions that return a
> > string.
> >
> > I'm not really a huge fan of this, but it's not a fault of this patch
> > series, so let's read on.
> 
> I concur.  "If they do not return any useful value, they should be
> void" was my first reaction, but presumably just like "return
> error("message");" is a handy way to give message while signalling
> an error to the caller, these are used to return NULL that signals
> an error?  I do not offhand think of a good longer-term direction to
> improve this one.

Yes, that's exactly the purpose. I don't see many changes that could
let it still fulfill that purpose, though perhaps one could argue that
it is unnecessarily confusing for the small shortening of the code it
provides (and ditto for error() itself).

There is one thing it probably could do: return a "void *" instead. That
would make it applicable to a wider variety of functions. But it also
makes it even more obscure (IMHO), and this is a static-local function
that is only used for functions that return strings anyway.

If we wanted to make it more generic (and I do not think we want to), we
can see that it differs from error() in two dimensions:

  - error() writes to stderr, but error_buf() writes to a strbuf (or
    nowhere if the strbuf is NULL)

  - error() passes along integer "-1" to signal error, but error_buf()
    passes along NULL

So of the four combinations, we have:

  - stderr / integer: error()
  - stderr / pointer: not implemented
  - strbuf / integer: not implemented
  - strbuf / pointer: error_buf()

One could imagine a suite of related functions: error_int(),
error_null(), error_buf_int(), error_buf_null() that provide all four.

But I do not see us clamoring to extend the pattern further. ;)

-Peff

Re: [PATCH 1/4] remote: return non-const pointer from error_buf()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jan 19, 2026 at 04:28:42PM -0800, Junio C Hamano wrote:
>
>> Patrick Steinhardt <ps@pks.im> writes:
>> 
>> > This function signature is indeed quite misleading, and I'd argue that
>> > it continues to be so even after the change. I guess the intent is to
>> > make it a bit easier to print an error in functions that return a
>> > string.
>> >
>> > I'm not really a huge fan of this, but it's not a fault of this patch
>> > series, so let's read on.
>> 
>> I concur.  "If they do not return any useful value, they should be
>> void" was my first reaction, but presumably just like "return
>> error("message");" is a handy way to give message while signalling
>> an error to the caller, these are used to return NULL that signals
>> an error?  I do not offhand think of a good longer-term direction to
>> improve this one.
>
> Yes, that's exactly the purpose. I don't see many changes that could
> let it still fulfill that purpose, though perhaps one could argue that
> it is unnecessarily confusing for the small shortening of the code it
> provides (and ditto for error() itself).
>
> There is one thing it probably could do: return a "void *" instead. That
> would make it applicable to a wider variety of functions. But it also
> makes it even more obscure (IMHO), and this is a static-local function
> that is only used for functions that return strings anyway.
>
> If we wanted to make it more generic (and I do not think we want to), we
> can see that it differs from error() in two dimensions:
>
>   - error() writes to stderr, but error_buf() writes to a strbuf (or
>     nowhere if the strbuf is NULL)
>
>   - error() passes along integer "-1" to signal error, but error_buf()
>     passes along NULL
>
> So of the four combinations, we have:
>
>   - stderr / integer: error()
>   - stderr / pointer: not implemented
>   - strbuf / integer: not implemented
>   - strbuf / pointer: error_buf()
>
> One could imagine a suite of related functions: error_int(),
> error_null(), error_buf_int(), error_buf_null() that provide all four.
>
> But I do not see us clamoring to extend the pattern further. ;)

;-).  Perhaps stop being cute and doing

	if (... error ...) {
		format_error(err, _("error message"), ...);
                return NULL;
	}

without any magic might be more appropriate for a file-scope static
that is only used for a handful of times?  I do not think the
situation is bad enough to warrant patch noise like this, but it is
sufficiently bad that I wish we wrote them in such a more trivial
way in the first place X-<.

[PATCH 2/4] remote: drop const return of tracking_for_push_dest()

[Jeff King]

The string returned from tracking_for_push_dest() comes from
apply_refspec(), and thus is always an allocated string (or NULL). We
should return a non-const pointer so that the caller knows that
ownership of the string is being transferred.

This goes back to the function's origin in e291c75a95 (remote.c: add
branch_get_push, 2015-05-21). It never really mattered because our
return is just forwarded through branch_get_push_1(), which returns a
const string as part of an intentionally hacky memory management scheme
(see that commit for details).

As the first step of untangling that hackery, let's drop the extra const
from this helper function (and from the variables that store its
result). There should be no functional change (yet).

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index 3dc100be83..5de9619bc7 100644
--- a/remote.c
+++ b/remote.c
@@ -1869,9 +1869,9 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
 	return branch->merge[0]->dst;
 }
 
-static const char *tracking_for_push_dest(struct remote *remote,
-					  const char *refname,
-					  struct strbuf *err)
+static char *tracking_for_push_dest(struct remote *remote,
+				    const char *refname,
+				    struct strbuf *err)
 {
 	char *ret;
 
@@ -1899,7 +1899,7 @@ static const char *branch_get_push_1(struct repository *repo,
 
 	if (remote->push.nr) {
 		char *dst;
-		const char *ret;
+		char *ret;
 
 		dst = apply_refspecs(&remote->push, branch->refname);
 		if (!dst)
@@ -1929,7 +1929,8 @@ static const char *branch_get_push_1(struct repository *repo,
 	case PUSH_DEFAULT_UNSPECIFIED:
 	case PUSH_DEFAULT_SIMPLE:
 		{
-			const char *up, *cur;
+			const char *up;
+			char *cur;
 
 			up = branch_get_upstream(branch, err);
 			if (!up)
-- 
2.53.0.rc0.338.g08aa8a9473

Re: [PATCH 2/4] remote: drop const return of tracking_for_push_dest()

[Patrick Steinhardt]

On Mon, Jan 19, 2026 at 12:20:26AM -0500, Jeff King wrote:
> The string returned from tracking_for_push_dest() comes from
> apply_refspec(), and thus is always an allocated string (or NULL). We
> should return a non-const pointer so that the caller knows that
> ownership of the string is being transferred.
> 
> This goes back to the function's origin in e291c75a95 (remote.c: add
> branch_get_push, 2015-05-21). It never really mattered because our
> return is just forwarded through branch_get_push_1(), which returns a
> const string as part of an intentionally hacky memory management scheme
> (see that commit for details).

Okay, so here we can now also return a `char *` now that `error_buf()`
got adapted.

> As the first step of untangling that hackery, let's drop the extra const
> from this helper function (and from the variables that store its
> result). There should be no functional change (yet).

Yup. The memory handling still feels weird, but as in the preceding
commit that's not a fault of this patch series.

Patrick

[PATCH 3/4] remote: fix leak in branch_get_push_1() with invalid "simple" config

[Jeff King]

Most of the code paths in branch_get_push_1() allocate a string for the
@{push} value. We then return the result, which is stored in a "struct
branch", so the value is not leaked.

But there's one path that does leak: when we are in the "simple" push
mode, we have to check that the @{push} value matches what we'd get for
@{upstream}. If it doesn't, we return an error, but forget to free the
@{push} value we computed.

Curiously, the existing tests don't trigger this with LSan, even though
they do exercise the code path. As far as I can tell, it should be
triggered via:

  git -c push.default=simple \
      -c branch.foo.remote=origin \
      -c branch.foo.merge=refs/heads/not-foo \
      rev-parse foo@{push}

which will complain that the upstream ("not-foo") does not match the
push destination ("foo"). We do die() shortly after this, but not until
after returning from branch_get_push_1(), which is where the leak
happens.

So it seems like a false negative in LSan. However, I can trigger it
reliably by printing the @{push} value using for-each-ref. This takes a
little more setup (because we need "foo" to actually exist to iterate
over it with for-each-ref), but we can piggy-back on the existing repo
config in t6300.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c                | 4 +++-
 t/for-each-ref-tests.sh | 9 +++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 5de9619bc7..e191b0ff6e 100644
--- a/remote.c
+++ b/remote.c
@@ -1938,9 +1938,11 @@ static const char *branch_get_push_1(struct repository *repo,
 			cur = tracking_for_push_dest(remote, branch->refname, err);
 			if (!cur)
 				return NULL;
-			if (strcmp(cur, up))
+			if (strcmp(cur, up)) {
+				free(cur);
 				return error_buf(err,
 						 _("cannot resolve 'simple' push to a single destination"));
+			}
 			return cur;
 		}
 	}
diff --git a/t/for-each-ref-tests.sh b/t/for-each-ref-tests.sh
index 4593be5fd5..bd2d45c971 100644
--- a/t/for-each-ref-tests.sh
+++ b/t/for-each-ref-tests.sh
@@ -1744,6 +1744,15 @@ test_expect_success ':remotename and :remoteref' '
 	)
 '
 
+test_expect_success '%(push) with an invalid push-simple config' '
+	echo "refs/heads/main " >expect &&
+	git -c push.default=simple \
+	    -c remote.pushdefault=myfork \
+	    for-each-ref \
+	    --format="%(refname) %(push)" refs/heads/main >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success "${git_for_each_ref} --ignore-case ignores case" '
 	${git_for_each_ref} --format="%(refname)" refs/heads/MAIN >actual &&
 	test_must_be_empty actual &&
-- 
2.53.0.rc0.338.g08aa8a9473

Re: [PATCH 3/4] remote: fix leak in branch_get_push_1() with invalid "simple" config

[Patrick Steinhardt]

On Mon, Jan 19, 2026 at 12:22:08AM -0500, Jeff King wrote:
> diff --git a/remote.c b/remote.c
> index 5de9619bc7..e191b0ff6e 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1938,9 +1938,11 @@ static const char *branch_get_push_1(struct repository *repo,
>  			cur = tracking_for_push_dest(remote, branch->refname, err);
>  			if (!cur)
>  				return NULL;
> -			if (strcmp(cur, up))
> +			if (strcmp(cur, up)) {
> +				free(cur);
>  				return error_buf(err,
>  						 _("cannot resolve 'simple' push to a single destination"));
> +			}

Yup, this memory leak was easy to spot in the preceding commit after
your refactorings.

Patrick

[PATCH 4/4] remote: always allocate branch.push_tracking_ref

[Jeff King]

In branch_get_push(), we usually allocate a new string for the @{push}
ref, but will not do so in push.default=upstream mode, where we just
pass back the result of branch_get_upstream() directly.

This led to a hacky memory management scheme in e291c75a95 (remote.c:
add branch_get_push, 2015-05-21): we store the result in the
push_tracking_ref field of a "struct branch", under the assumption that
the branch struct will last until the end of the program. So even though
the struct doesn't know if it has an allocated string or not, it doesn't
matter because we hold on to it either way.

But that assumption was violated by f5ccb535cc (remote: fix leaking
config strings, 2024-08-22), which added a function to free branch
structs. Any struct which is fed to branch_release() is at risk of
leaking its push_tracking_ref member.

I don't think this can actually be triggered in practice. We rarely
actually free the branch structs, and we only fill in the
push_tracking_ref string lazily when it is needed. So triggering the
leak would require a code path that does both, and I couldn't find one.

Still, this is an ugly trap that may eventually spring on us. Since
there is only one code path in branch_get_push() that doesn't allocate,
let's just have it copy the string. And then we know that
push_tracking_ref is always allocated, and we can free it in
branch_release().

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 7 ++++---
 remote.h | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/remote.c b/remote.c
index e191b0ff6e..3e9d9b3e1f 100644
--- a/remote.c
+++ b/remote.c
@@ -272,6 +272,7 @@ static void branch_release(struct branch *branch)
 	free((char *)branch->refname);
 	free(branch->remote_name);
 	free(branch->pushremote_name);
+	free(branch->push_tracking_ref);
 	merge_clear(branch);
 }
 
@@ -1883,8 +1884,8 @@ static char *tracking_for_push_dest(struct remote *remote,
 	return ret;
 }
 
-static const char *branch_get_push_1(struct repository *repo,
-				     struct branch *branch, struct strbuf *err)
+static char *branch_get_push_1(struct repository *repo,
+			       struct branch *branch, struct strbuf *err)
 {
 	struct remote_state *remote_state = repo->remote_state;
 	struct remote *remote;
@@ -1924,7 +1925,7 @@ static const char *branch_get_push_1(struct repository *repo,
 		return tracking_for_push_dest(remote, branch->refname, err);
 
 	case PUSH_DEFAULT_UPSTREAM:
-		return branch_get_upstream(branch, err);
+		return xstrdup_or_null(branch_get_upstream(branch, err));
 
 	case PUSH_DEFAULT_UNSPECIFIED:
 	case PUSH_DEFAULT_SIMPLE:
diff --git a/remote.h b/remote.h
index 0ca399e183..fc052945ee 100644
--- a/remote.h
+++ b/remote.h
@@ -331,7 +331,7 @@ struct branch {
 
 	int merge_alloc;
 
-	const char *push_tracking_ref;
+	char *push_tracking_ref;
 };
 
 struct branch *branch_get(const char *name);
-- 
2.53.0.rc0.338.g08aa8a9473

Re: [PATCH 4/4] remote: always allocate branch.push_tracking_ref

[Patrick Steinhardt]

On Mon, Jan 19, 2026 at 12:23:20AM -0500, Jeff King wrote:
> diff --git a/remote.c b/remote.c
> index e191b0ff6e..3e9d9b3e1f 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1924,7 +1925,7 @@ static const char *branch_get_push_1(struct repository *repo,
>  		return tracking_for_push_dest(remote, branch->refname, err);
>  
>  	case PUSH_DEFAULT_UPSTREAM:
> -		return branch_get_upstream(branch, err);
> +		return xstrdup_or_null(branch_get_upstream(branch, err));
>  
>  	case PUSH_DEFAULT_UNSPECIFIED:
>  	case PUSH_DEFAULT_SIMPLE:

Makes sense. I was wondering whether you'd also change
`branch_get_push_1()` in a subsequent patch, so I'm happy to see this.

This whole series looks good to me, thanks!

Patrick

Triangular workflow

[Harald Nordgren]

Thanks a lot Jeff!

Would be nice to get this merged ASAP, so I can continue the work on my
feature without the memory leak there.


Harald

Re: Triangular workflow

[Jeff King]

On Mon, Jan 19, 2026 at 04:04:13PM +0100, Harald Nordgren wrote:

> Would be nice to get this merged ASAP, so I can continue the work on my
> feature without the memory leak there.

I imagine it will not get merged until after the upcoming release. The
usual thing is to build your branch on top in the meantime, though I
don't offhand know how good GGG's support is for then sending only your
patches (you might need to build your PR against the branch that Junio
created when he picked up the topic, but that is only in gitster/git,
not git/git).

-Peff

Re: [PATCH 0/4] memory leaks in remote.c

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This fixes some memory leaks in remote.c. Not urgent, as they are quite
> old, but they are newly triggered in the test suite by Harald's
> hn/status-compare-with-push topic. So I think we'd want to build that
> topic on top of these.
>
> The first two are just preparatory cleanups. Patch 3 fixes the leak that
> Harald's series triggers (and adds its own test, of course). Patch 4 is
> a hypothetical leak that I don't think can be triggered in practice (so
> it's more of a cleanup).
>
>   [1/4]: remote: return non-const pointer from error_buf()
>   [2/4]: remote: drop const return of tracking_for_push_dest()
>   [3/4]: remote: fix leak in branch_get_push_1() with invalid "simple" config
>   [4/4]: remote: always allocate branch.push_tracking_ref
>
>  remote.c                | 24 ++++++++++++++----------
>  remote.h                |  2 +-
>  t/for-each-ref-tests.sh |  9 +++++++++
>  3 files changed, 24 insertions(+), 11 deletions(-)

All look sensible.  Will queue.  Thanks.