patch, 4 partsmemory leaks in remote.c

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

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(-)

On Mon, Jan 19, 2026 at 12:19:45AM -0500, Jeff King wrote:

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

Patrick Steinhardt <ps@pks.im> writes:

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.

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

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

Jeff King <peff@peff.net> writes:

;-).  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-<.

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(-)

On Mon, Jan 19, 2026 at 12:20:26AM -0500, Jeff King wrote:

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

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

Patrick

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(-)

On Mon, Jan 19, 2026 at 12:22:08AM -0500, Jeff King wrote:

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

Patrick

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(-)

On Mon, Jan 19, 2026 at 12:23:20AM -0500, Jeff King wrote:

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

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

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

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

Jeff King <peff@peff.net> writes:

All look sensible.  Will queue.  Thanks.