Skip to content

Commit 9bf9eed

Browse files
peffgitster
authored andcommitted
remote: fix leak in branch_get_push_1() with invalid "simple" config
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
1 parent 782a719 commit 9bf9eed

2 files changed

Lines changed: 12 additions & 1 deletion

File tree

remote.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1945,9 +1945,11 @@ static const char *branch_get_push_1(struct repository *repo,
19451945
cur = tracking_for_push_dest(remote, branch->refname, err);
19461946
if (!cur)
19471947
return NULL;
1948-
if (strcmp(cur, up))
1948+
if (strcmp(cur, up)) {
1949+
free(cur);
19491950
return error_buf(err,
19501951
_("cannot resolve 'simple' push to a single destination"));
1952+
}
19511953
return cur;
19521954
}
19531955
}

t/for-each-ref-tests.sh

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1744,6 +1744,15 @@ test_expect_success ':remotename and :remoteref' '
17441744
)
17451745
'
17461746

1747+
test_expect_success '%(push) with an invalid push-simple config' '
1748+
echo "refs/heads/main " >expect &&
1749+
git -c push.default=simple \
1750+
-c remote.pushdefault=myfork \
1751+
for-each-ref \
1752+
--format="%(refname) %(push)" refs/heads/main >actual &&
1753+
test_cmp expect actual
1754+
'
1755+
17471756
test_expect_success "${git_for_each_ref} --ignore-case ignores case" '
17481757
${git_for_each_ref} --format="%(refname)" refs/heads/MAIN >actual &&
17491758
test_must_be_empty actual &&

0 commit comments

Comments
 (0)