tests: mark as passing with SANITIZE=leak

The goal in this series is to pass t3200 with SANITIZE=leak.

As a result of the fixes, other tests also pass.

This is the list of tests that no longer trigger any leak after this series:

   + t1507-rev-parse-upstream.sh
   + t1508-at-combinations.sh
   + t1514-rev-parse-push.sh
   + t2027-checkout-track.sh
   + t3200-branch.sh
   + t3204-branch-name-interpretation.sh
   + t5404-tracking-branches.sh
   + t5517-push-mirror.sh
   + t5525-fetch-tagopt.sh
   + t6040-tracking-info.sh
   + t7508-status.sh

Each of the commits (except 11/11) fixes a leak. They have no dependencies on each other. As a result, they can be reordered.

To review one leak, the commit can be moved to the tip or reverted. E.g. to review: "branch: fix a leak in check_tracking_branch", this can be used:

  $ git revert --no-edit HEAD~3
  $ make SANITIZE=leak test T=t3200-branch.sh 

Also, each commit have a minimal script in the message that can be used to reproduce the leak.

Rubén Justo (11):
  rev-parse: fix a leak with --abbrev-ref
  config: fix a leak in git_config_copy_or_rename_section_in_file
  remote: fix a leak in query_matches_negative_refspec
  branch: fix a leak in dwim_and_setup_tracking
  branch: fix a leak in setup_tracking
  branch: fix a leak in cmd_branch
  branch: fix a leak in inherit_tracking
  branch: fix a leak in check_tracking_branch
  branch: fix a leak in setup_tracking
  config: fix a leak in git_config_copy_or_rename_section_in_file
  tests: mark as passing with SANITIZE=leak

 branch.c                              | 14 +++++++++-----
 builtin/branch.c                      |  2 +-
 builtin/rev-parse.c                   |  7 +++++--
 config.c                              |  2 ++
 remote.c                              |  2 +-
 t/t1507-rev-parse-upstream.sh         |  1 +
 t/t1508-at-combinations.sh            |  1 +
 t/t1514-rev-parse-push.sh             |  1 +
 t/t2027-checkout-track.sh             |  1 +
 t/t3200-branch.sh                     |  1 +
 t/t3204-branch-name-interpretation.sh |  1 +
 t/t5404-tracking-branches.sh          |  1 +
 t/t5517-push-mirror.sh                |  1 +
 t/t5525-fetch-tagopt.sh               |  1 +
 t/t6040-tracking-info.sh              |  1 +
 t/t7508-status.sh                     |  1 +
 16 files changed, 29 insertions(+), 9 deletions(-)

To handle "--abbrev-ref" we use shorten_unambiguous_ref(). This function takes a refname and returns a shortened refname, which is a newly allocated string that needs to be freed.

Unfortunately, the refname variable is reused to receive the shortened one. Therefore, we lose the original refname, which needs to be freed as well, producing a leak.

This leak can be reviewed with:

   $ for a in {1..10}; do git branch foo_${a}; done
   $ git rev-parse --abbrev-ref refs/heads/foo_{1..10}

   Direct leak of 171 byte(s) in 10 object(s) allocated from:
       ... in xstrdup wrapper.c
       ... in expand_ref refs.c
       ... in repo_dwim_ref refs.c
       ... in show_rev builtin/rev-parse.c
       ... in cmd_rev_parse builtin/rev-parse.c
       ... in run_builtin git.c

We have this leak since a45d34691e (rev-parse: --abbrev-ref option to shorten ref name, 2009-04-13) when "--abbrev-ref" was introduced.

Let's fix it.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/rev-parse.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

On Sun, Jun 11, 2023 at 08:49:17PM +0200, Rubén Justo wrote:

Makes sense, but...

The lifetime of "to_free" is much longer than it needs to be here. We'd usually use a "to_free" pattern when the memory is aliased to another (usually const) pointer, and the allocation needs to live as long as that other pointer. But here, nobody cares about the old value as soon as we replace it. We could almost just free() it ahead of time, but of course we also need to pass it to the shortening function. So maybe:

On 11-jun-2023 23:12:39, Jeff King wrote:

Simpler.  Nice.

Thanks!

In 52d59cc645 (branch: add a --copy (-c) option to go with --move (-m), 2017-06-18) a new strbuf variable was introduced, but not released.

Thus, when copying a branch that has any configuration, we have a leak.

   $ git branch foo
   $ git config branch.foo.some-key some_value
   $ git branch -c foo bar

   Direct leak of 65 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in strbuf_grow strbuf.c
       ... in strbuf_vaddf strbuf.c
       ... in strbuf_addf strbuf.c
       ... in store_create_section config.c
       ... in git_config_copy_or_rename_section_in_file config.c
       ... in git_config_copy_section_in_file config.c
       ... in git_config_copy_section config.c
       ... in copy_or_rename_branch builtin/branch.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

Let's fix that leak.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 config.c | 1 +
 1 file changed, 1 insertion(+)

On Sun, Jun 11, 2023 at 08:49:28PM +0200, Rubén Justo wrote:

Looks good. Thanks for digging up the commit which introduced the problem. I always find that gives me more confidence that the leak is just a simple "oops, we forgot to free", and there isn't something more subtle going on.

-Peff

In c0192df630 (refspec: add support for negative refspecs, 2020-09-30) query_matches_negative_refspec() was introduced.

The function was implemented as a two-loop process, where the former loop accumulates and the latter evaluates. To accumulate, a string_list is used.

Within the first loop, there are three cases where a string is added to the string_list. Two of them add strings that do not need to be freed. But in the third case, the string added is returned by match_name_with_pattern(), which needs to be freed.

The string_list is initialized with STRING_LIST_INIT_NODUP, i.e. when cleared, the strings added are not freed. Therefore, the string returned by match_name_with_pattern() is not freed, so we have a leak.

   $ git remote add local .
   $ git update-ref refs/remotes/local/foo HEAD
   $ git branch --track bar local/foo

   Direct leak of 24 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in strbuf_grow strbuf.c
       ... in strbuf_add strbuf.c
       ... in match_name_with_pattern remote.c
       ... in query_matches_negative_refspec remote.c
       ... in query_refspecs remote.c
       ... in remote_find_tracking remote.c
       ... in find_tracked_branch branch.c
       ... in for_each_remote remote.c
       ... in setup_tracking branch.c
       ... in create_branch branch.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

   Direct leak of 24 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in strbuf_grow strbuf.c
       ... in strbuf_add strbuf.c
       ... in match_name_with_pattern remote.c
       ... in query_matches_negative_refspec remote.c
       ... in query_refspecs remote.c
       ... in remote_find_tracking remote.c
       ... in check_tracking_branch branch.c
       ... in for_each_remote remote.c
       ... in validate_remote_tracking_branch branch.c
       ... in dwim_branch_start branch.c
       ... in create_branch branch.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

An interesting point to note is that while string_list_append() is used in the first two cases described, string_list_append_nodup() is used in the third. This seems to indicate an intention to delegate the responsibility for freeing the string, to the string_list. As if the string_list had been initialized with STRING_LIST_INIT_DUP, i.e. the strings are strdup()'d when added (except if the "_nodup" API is used) and freed when cleared.

Switching to STRING_LIST_INIT_DUP fixes the leak and probably is what we wanted to do originally. Let's do it.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Sun, Jun 11, 2023 at 08:49:35PM +0200, Rubén Justo wrote:

Ah, clearing a NODUP string-list strikes again. :) I concur with your reasoning here; switching it to DUP is the best fix.

-Peff

On 11-jun-2023 23:17:32, Jeff King wrote:

Yes.  "_nodup" with NODUP string_list, shows red flags, imo.

Thank you for reviewing the change.

In e89f151db1 (branch: move --set-upstream-to behavior to dwim_and_setup_tracking(), 2022-01-28) the string returned by dwim_branch_start() was not freed, resulting in a memory leak.

It can be reviewed with:

   $ git remote add local .
   $ git update-ref refs/remotes/local/foo HEAD
   $ git branch --set-upstream-to local/foo foo

   Direct leak of 23 byte(s) in 1 object(s) allocated from:
       ... in xstrdup wrapper.c
       ... in expand_ref refs.c
       ... in repo_dwim_ref refs.c
       ... in dwim_branch_start branch.c
       ... in dwim_and_setup_tracking branch.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

Let's free it now.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 branch.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

On Sun, Jun 11, 2023 at 08:49:43PM +0200, Rubén Justo wrote:

Yep, looks good. One small comment:

Adding the NULL-initialization signals that we expect that real_orig_ref might sometimes _not_ be filled in by dwim_branch_start(). But I think it always will be; and if it weren't, that would make passing it to setup_tracking() rather suspicious.

So I don't think the NULL initialization is wrong, and certainly it is more defensive. But I find it a little misleading.

-Peff

On 11-jun-2023 23:21:06, Jeff King wrote:

I don't have any objection to this. But I've seen Junio has already merged this to 'next'. I'm not going to re-roll this commit. Sorry.

Thank you for your review.

In bdaf1dfae7 (branch: new autosetupmerge option "simple" for matching branches, 2022-04-29) a new exit for setup_tracking() missed the clean-up, producing a leak.

   $ git config branch.autoSetupMerge simple
   $ git remote add local .
   $ git update-ref refs/remotes/local/foo HEAD
   $ git branch bar local/foo

   Direct leak of 384 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in string_list_append_nodup string-list.c
       ... in find_tracked_branch branch.c
       ... in for_each_remote remote.c
       ... in setup_tracking branch.c
       ... in create_branch branch.c
       ... in cmd_branch builtinbranch.c
       ... in run_builtin git.c

   Indirect leak of 24 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in strbuf_grow strbuf.c
       ... in strbuf_add strbuf.c
       ... in match_name_with_pattern remote.c
       ... in query_refspecs remote.c
       ... in remote_find_tracking remote.c
       ... in find_tracked_branch branch.c
       ... in for_each_remote remote.c
       ... in setup_tracking branch.c
       ... in create_branch branch.c
       ... in cmd_branch builtinbranch.c
       ... in run_builtin git.c

The return introduced in bdaf1dfae7 was to avoid setting up the tracking, but even in that case it is still necessary to do the clean-up. Let's do it.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Sun, Jun 11, 2023 at 08:49:51PM +0200, Rubén Justo wrote:

That may be a sign that the return introduced by that commit is in the wrong spot (i.e., could we be checking it earlier and returning before doing the work that led to the allocations?).

But I didn't look too carefully, and I think for the purposes of your series it is OK to simply fix the leak without digging too far.

I'll cc the author (and quote the patch below) though, as sometimes in cases like these they may be interested in looking deeper themselves.

-Peff

On 11-jun-2023 23:26:15, Jeff King wrote:

Thanks.

Perfect.  Thank you.

In 98e7ab6d42 (for-each-ref: delay parsing of --sort=<atom> options, 2021-10-20) a new string_list was introduced to accumulate any "branch.sort" setting.

That string_list is cleared in ref_sorting_options(), which is only called when processing the "--list" sub-command. Therefore, with other sub-command, while having any sort option set, a leak is produced, e.g.:

   $ git config branch.sort invalid_sort_option
   $ git branch --edit-description

   Direct leak of 384 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in string_list_append_nodup string-list.c
       ... in string_list_append string-list.c
       ... in git_branch_config builtin/branch.c
       ... in configset_iter config.c
       ... in repo_config config.c
       ... in git_config config.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

   Indirect leak of 20 byte(s) in 1 object(s) allocated from:
       ... in xstrdup wrapper.c
       ... in string_list_append string-list.c
       ... in git_branch_config builtin/branch.c
       ... in configset_iter config.c
       ... in repo_config config.c
       ... in git_config config.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

We don't have a common clean-up section in cmd_branch(). To avoid refactoring and keep the fix simple, and while we find a better solution, let's silence the leak-hunter making the list static.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Sun, Jun 11, 2023 at 08:50:05PM +0200, Rubén Justo wrote:

Gross. :)

If we are just going to annotate here, I'd prefer to use UNLEAK(). It makes it more obvious this is about leak-checking, and doesn't imply that we otherwise care about the lifetime of this field. Like:

On 11-jun-2023 23:46:39, Jeff King wrote:

XD

OK.  Will do this.

Thank you for reviewing the change with a wide view.

In d3115660b4 (branch: add flags and config to inherit tracking, 2021-12-20) a new option was introduced to allow creating a new branch, inheriting the tracking of another branch.

The new code, strdup()'d the remote_name of the existing branch, but unfortunately it was not freed, producing a leak.

   $ git remote add local .
   $ git update-ref refs/remotes/local/foo HEAD
   $ git branch --track bar local/foo
   branch 'bar' set up to track 'local/foo'.
   $ git branch --track=inherit baz bar
   branch 'baz' set up to track 'local/foo'.

   Direct leak of 6 byte(s) in 1 object(s) allocated from:
       ... in xstrdup wrapper.c
       ... in inherit_tracking branch.c
       ... in setup_tracking branch.c
       ... in create_branch branch.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

Actually, the string we're strdup()'ing is from the struct branch returned by get_branch(). Which, in turn, retrieves the string from the global "struct repository". This makes perfectly valid to use the string throughout the entire execution of create_branch(). There is no need to duplicate it.

Let's fix the leak, removing the strdup().

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Sun, Jun 11, 2023 at 08:50:16PM +0200, Rubén Justo wrote:

Yep, good reasoning. I agree this is a good way to fix the leak.

-Peff

Let's fix a leak we have in check_tracking_branch() since the function was introduced in 41c21f22d0 (branch.c: Validate tracking branches with refspecs instead of refs/remotes/*, 2013-04-21).

The leak can be reviewed with:

   $ git remote add local .
   $ git update-ref refs/remotes/local/foo HEAD
   $ git branch --track bar local/foo

   Direct leak of 24 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in strbuf_grow strbuf.c
       ... in strbuf_add strbuf.c
       ... in match_name_with_pattern remote.c
       ... in query_refspecs remote.c
       ... in remote_find_tracking remote.c
       ... in check_tracking_branch branch.c
       ... in for_each_remote remote.c
       ... in validate_remote_tracking_branch branch.c
       ... in dwim_branch_start branch.c
       ... in create_branch branch.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 branch.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

On Sun, Jun 11, 2023 at 08:50:27PM +0200, Rubén Justo wrote:

OK, so we expect remote_find_tracking() to fill in the "src" field, but we don't actually care about the value here (we are just validating). It probably doesn't fill in the value for some error cases, but then we'd be left with a NULL, which is OK to feed to free(). Makes sense. I double-checked that it always allocates "src" when it is assigned to, so I think this fix is good.

-Peff

The commit d3115660b4 (branch: add flags and config to inherit tracking, 2021-12-20) replaced in "struct tracking", the member "char *src" by a new "struct string_list *srcs".

This caused a modification in find_tracked_branch(). The string returned by remote_find_tracking(), previously assigned to "src", is now added to the string_list "srcs".

That string_list is initialized with STRING_LIST_INIT_DUP, which means that what is added is not the given string, but a duplicate. Therefore, the string returned by remote_find_tracking() is leaked.

The leak can be reviewed with:

   $ git branch foo
   $ git remote add local .
   $ git fetch local
   $ git branch --track bar local/foo

   Direct leak of 24 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in strbuf_grow strbuf.c
       ... in strbuf_add strbuf.c
       ... in match_name_with_pattern remote.c
       ... in query_refspecs remote.c
       ... in remote_find_tracking remote.c
       ... in find_tracked_branch branch.c
       ... in for_each_remote remote.c
       ... in setup_tracking branch.c
       ... in create_branch branch.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

Let's fix the leak, using the "_nodup" API to add to the string_list. This way, the string itself will be added instead of being strdup()'d. And when the string_list is cleared, the string will be freed.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Sun, Jun 11, 2023 at 08:50:36PM +0200, Rubén Justo wrote:

Your fix makes sense. I had to stare at the existing code for a long time to make sure the _other_ side of the switch wasn't leaking, but it works by falling through "case 2" into "default", which frees tracking->spec.src. So this should plug the leak completely.

-Peff

A branch can have its configuration spread over several configuration sections. This situation was already foreseen in 52d59cc645 (branch: add a --copy (-c) option to go with --move (-m), 2017-06-18) when "branch -c" was introduced.

Unfortunately, a leak was also introduced:

   $ git branch foo
   $ cat >> .git/config <<EOF
   [branch "foo"]
   	some-key-a = a value
   [branch "foo"]
   	some-key-b = b value
   [branch "foo"]
   	some-key-c = c value
   EOF
   $ git branch -c foo bar

   Direct leak of 130 byte(s) in 2 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in strbuf_grow strbuf.c
       ... in strbuf_vaddf strbuf.c
       ... in strbuf_addf strbuf.c
       ... in store_create_section config.c
       ... in git_config_copy_or_rename_section_in_file config.c
       ... in git_config_copy_section_in_file config.c
       ... in git_config_copy_section config.c
       ... in copy_or_rename_branch builtin/branch.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

Let's fix it.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 config.c | 1 +
 1 file changed, 1 insertion(+)

On Sun, Jun 11, 2023 at 08:50:45PM +0200, Rubén Justo wrote:

Wow, I did a double-take on this code. It is uncommon in our codebase to assign a struct by value in a function return like this, and doubly weird to assign a strbuf (since the whole point of strbuf is to use the opaque functions to make sure we aren't overwriting existing allocations or aliasing pointers).

I think your fix here is the correct thing if we aren't going to clean up the code further.

The more usual thing for our codebase would be refactoring like:

On 12-jun-2023 00:05:18, Jeff King wrote:

I have a draft with the exact same change. I chose the simple fix, though. I wasn't expecting your review :)

I'll definitely re-roll with this.

Thanks!

The tests listed below, since previous commits, no longer trigger any leak.

   + t1507-rev-parse-upstream.sh
   + t1508-at-combinations.sh
   + t1514-rev-parse-push.sh
   + t2027-checkout-track.sh
   + t3200-branch.sh
   + t3204-branch-name-interpretation.sh
   + t5404-tracking-branches.sh
   + t5517-push-mirror.sh
   + t5525-fetch-tagopt.sh
   + t6040-tracking-info.sh
   + t7508-status.sh

Let's mark them with "TEST_PASSES_SANITIZE_LEAK=true" to notice and fix promptly any new leak that may be introduced and triggered by them, in the future.

As a reference, the list has been formed using:

  $ GIT_TEST_PASSING_SANITIZE_LEAK=check make SANITIZE=leak test

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t1507-rev-parse-upstream.sh         | 1 +
 t/t1508-at-combinations.sh            | 1 +
 t/t1514-rev-parse-push.sh             | 1 +
 t/t2027-checkout-track.sh             | 1 +
 t/t3200-branch.sh                     | 1 +
 t/t3204-branch-name-interpretation.sh | 1 +
 t/t5404-tracking-branches.sh          | 1 +
 t/t5517-push-mirror.sh                | 1 +
 t/t5525-fetch-tagopt.sh               | 1 +
 t/t6040-tracking-info.sh              | 1 +
 t/t7508-status.sh                     | 1 +
 11 files changed, 11 insertions(+)

On 11-jun-2023 20:29:09, Rubén Justo wrote:

Sorry.  This should have been:

[PATCH 00/11] tests: mark as passing with SANITIZE=leak

I'll resend with the correct subject, but will wait some time for
reviews.
 

On Sun, Jun 11, 2023 at 08:29:09PM +0200, Rubén Justo wrote:

Thanks for a pleasant read. I think each case is correct, though I left a few minor comments. Some of them are things I think are worth including in a re-roll. Others are things that we _could_ go further to refactor for clarity, but I am OK if you want to stay focused on the minimal leak-fixes.

-Peff

On 12/6/23 6:06, Jeff King wrote:

Thank you for your prompt reviews, and sorry for my late responses.

Rubén Justo <rjusto@gmail.com> writes:

Thanks for working on these. I've seen Peff's reviews and picked up only the ones that are unambiguously good, dropping the rest that you may want to update. And as a consequence, 11/11 is also not included in the set that has been queued. Please do include that final step when you resend updated bits that I dropped here.

Thanks.

The goal in this series is to pass t3200 with SANITIZE=leak.

As a result of the fixes, other tests also pass.

This is the list of tests that no longer trigger any leak after this series:

   + t1507-rev-parse-upstream.sh
   + t1508-at-combinations.sh
   + t1514-rev-parse-push.sh
   + t2027-checkout-track.sh
   + t3200-branch.sh
   + t3204-branch-name-interpretation.sh
   + t5404-tracking-branches.sh
   + t5517-push-mirror.sh
   + t5525-fetch-tagopt.sh
   + t6040-tracking-info.sh
   + t7508-status.sh

Each of the commits (except 5/5) fixes a leak. They have no dependencies on each other. As a result, they can be reordered.

To review one leak, the commit can be moved to the tip or reverted. E.g. to review: "branch: fix a leak in setup_tracking", this can be used:

  $ git revert --no-edit HEAD~3
  $ make SANITIZE=leak test T=t3200-branch.sh

Also, each commit have a minimal script in the message that can be used to reproduce the leak.

This is the second version of this series. However, a subset of the patches from the first version have already been merged to 'next'. Therefore, those are not included here.

These are the rest of them, which address Peff's reviews.

Thanks.

Rubén Justo (5):
  rev-parse: fix a leak with --abbrev-ref
  branch: fix a leak in setup_tracking
  branch: fix a leak in cmd_branch
  config: fix a leak in git_config_copy_or_rename_section_in_file
  tests: mark as passing with SANITIZE=leak

 branch.c                              |  2 +-
 builtin/branch.c                      |  2 ++
 builtin/rev-parse.c                   |  5 ++++-
 config.c                              | 29 ++++++++++++++-------------
 t/t1507-rev-parse-upstream.sh         |  1 +
 t/t1508-at-combinations.sh            |  1 +
 t/t1514-rev-parse-push.sh             |  1 +
 t/t2027-checkout-track.sh             |  1 +
 t/t3200-branch.sh                     |  1 +
 t/t3204-branch-name-interpretation.sh |  1 +
 t/t5404-tracking-branches.sh          |  1 +
 t/t5517-push-mirror.sh                |  1 +
 t/t5525-fetch-tagopt.sh               |  1 +
 t/t6040-tracking-info.sh              |  1 +
 t/t7508-status.sh                     |  1 +
 15 files changed, 33 insertions(+), 16 deletions(-)

To handle "--abbrev-ref" we use shorten_unambiguous_ref(). This function takes a refname and returns a shortened refname, which is a newly allocated string that needs to be freed.

Unfortunately, the refname variable is reused to receive the shortened one. Therefore, we lose the original refname, which needs to be freed as well, producing a leak.

This leak can be reviewed with:

   $ for a in {1..10}; do git branch foo_${a}; done
   $ git rev-parse --abbrev-ref refs/heads/foo_{1..10}

   Direct leak of 171 byte(s) in 10 object(s) allocated from:
       ... in xstrdup wrapper.c
       ... in expand_ref refs.c
       ... in repo_dwim_ref refs.c
       ... in show_rev builtin/rev-parse.c
       ... in cmd_rev_parse builtin/rev-parse.c
       ... in run_builtin git.c

We have this leak since a45d34691e (rev-parse: --abbrev-ref option to shorten ref name, 2009-04-13) when "--abbrev-ref" was introduced.

Let's fix it.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/rev-parse.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

In bdaf1dfae7 (branch: new autosetupmerge option "simple" for matching branches, 2022-04-29) a new exit for setup_tracking() missed the clean-up, producing a leak.

   $ git config branch.autoSetupMerge simple
   $ git remote add local .
   $ git update-ref refs/remotes/local/foo HEAD
   $ git branch bar local/foo

   Direct leak of 384 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in string_list_append_nodup string-list.c
       ... in find_tracked_branch branch.c
       ... in for_each_remote remote.c
       ... in setup_tracking branch.c
       ... in create_branch branch.c
       ... in cmd_branch builtinbranch.c
       ... in run_builtin git.c

   Indirect leak of 24 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in strbuf_grow strbuf.c
       ... in strbuf_add strbuf.c
       ... in match_name_with_pattern remote.c
       ... in query_refspecs remote.c
       ... in remote_find_tracking remote.c
       ... in find_tracked_branch branch.c
       ... in for_each_remote remote.c
       ... in setup_tracking branch.c
       ... in create_branch branch.c
       ... in cmd_branch builtinbranch.c
       ... in run_builtin git.c

The return introduced in bdaf1dfae7 was to avoid setting up the tracking, but even in that case it is still necessary to do the clean-up. Let's do it.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

In 98e7ab6d42 (for-each-ref: delay parsing of --sort=<atom> options, 2021-10-20) a new string_list was introduced to accumulate any "branch.sort" setting.

That string_list is cleared in ref_sorting_options(), which is only called when processing the "--list" sub-command. Therefore, with other sub-command, while having any sort option set, a leak is produced, e.g.:

   $ git config branch.sort invalid_sort_option
   $ git branch --edit-description

   Direct leak of 384 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in string_list_append_nodup string-list.c
       ... in string_list_append string-list.c
       ... in git_branch_config builtin/branch.c
       ... in configset_iter config.c
       ... in repo_config config.c
       ... in git_config config.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

   Indirect leak of 20 byte(s) in 1 object(s) allocated from:
       ... in xstrdup wrapper.c
       ... in string_list_append string-list.c
       ... in git_branch_config builtin/branch.c
       ... in configset_iter config.c
       ... in repo_config config.c
       ... in git_config config.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

We don't have a common clean-up section in cmd_branch(). To avoid refactoring, keep the fix simple, and while we find a better solution which hopefuly will avoid entirely that string_list, when no sort options are needed; let's squelch the leak sanitizer using UNLEAK().

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c | 2 ++
 1 file changed, 2 insertions(+)

A branch can have its configuration spread over several configuration sections. This situation was already foreseen in 52d59cc645 (branch: add a --copy (-c) option to go with --move (-m), 2017-06-18) when "branch -c" was introduced.

Unfortunately, a leak was also introduced:

   $ git branch foo
   $ cat >> .git/config <<EOF
   [branch "foo"]
   	some-key-a = a value
   [branch "foo"]
   	some-key-b = b value
   [branch "foo"]
   	some-key-c = c value
   EOF
   $ git branch -c foo bar

   Direct leak of 130 byte(s) in 2 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in strbuf_grow strbuf.c
       ... in strbuf_vaddf strbuf.c
       ... in strbuf_addf strbuf.c
       ... in store_create_section config.c
       ... in git_config_copy_or_rename_section_in_file config.c
       ... in git_config_copy_section_in_file config.c
       ... in git_config_copy_section config.c
       ... in copy_or_rename_branch builtin/branch.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

Let's fix it.

Let's also modify the function store_create_section() so that it stops returning a strbuf, which is an uncommon pattern in our code base, and instead, starts receiving the strbuf to use from its caller.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 config.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

The tests listed below, since previous commits, no longer trigger any leak.

   + t1507-rev-parse-upstream.sh
   + t1508-at-combinations.sh
   + t1514-rev-parse-push.sh
   + t2027-checkout-track.sh
   + t3200-branch.sh
   + t3204-branch-name-interpretation.sh
   + t5404-tracking-branches.sh
   + t5517-push-mirror.sh
   + t5525-fetch-tagopt.sh
   + t6040-tracking-info.sh
   + t7508-status.sh

Let's mark them with "TEST_PASSES_SANITIZE_LEAK=true" to notice and fix promptly any new leak that may be introduced and triggered by them in the future.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t1507-rev-parse-upstream.sh         | 1 +
 t/t1508-at-combinations.sh            | 1 +
 t/t1514-rev-parse-push.sh             | 1 +
 t/t2027-checkout-track.sh             | 1 +
 t/t3200-branch.sh                     | 1 +
 t/t3204-branch-name-interpretation.sh | 1 +
 t/t5404-tracking-branches.sh          | 1 +
 t/t5517-push-mirror.sh                | 1 +
 t/t5525-fetch-tagopt.sh               | 1 +
 t/t6040-tracking-info.sh              | 1 +
 t/t7508-status.sh                     | 1 +
 11 files changed, 11 insertions(+)

Rubén Justo <rjusto@gmail.com> writes:

Thanks!

On Sat, Jun 17, 2023 at 01:27:47AM +0200, Rubén Justo wrote:

These all look good to me. I'd probably have split the config.c refactor into its own commit, but I don't think it's worth going back for.

Thanks Rubén for fixing these!

-Peff

On 17/6/23 7:48, Jeff King wrote:

That's a good point. I'll send the refactor when the dust settles, along with a revision for the UNLEAK().

Thank you!

The goal in this series is to pass t3200 with SANITIZE=leak.

As a result of the fixes, other tests also pass.

This is the list of tests that no longer trigger any leak after this series:

   + t1507-rev-parse-upstream.sh
   + t1508-at-combinations.sh
   + t1514-rev-parse-push.sh
   + t2027-checkout-track.sh
   + t3200-branch.sh
   + t3204-branch-name-interpretation.sh
   + t5404-tracking-branches.sh
   + t5517-push-mirror.sh
   + t5525-fetch-tagopt.sh
   + t6040-tracking-info.sh
   + t7508-status.sh

Each of the commits (except 5/5) fixes a leak. They have no dependencies on each other. As a result, they can be reordered.

To review one leak, the commit can be moved to the tip or reverted. E.g. to review: "branch: fix a leak in setup_tracking", this can be used:

  $ git revert --no-edit HEAD~3
  $ make SANITIZE=leak test T=t3200-branch.sh

Also, each commit have a minimal script in the message that can be used to reproduce the leak.

This is the third version of this series. The refactoring in config.c has been reverted, and will be sent outside of this series.

Sorry for the noise of a quick re-roll.

Thanks.

Rubén Justo (5):
  rev-parse: fix a leak with --abbrev-ref
  branch: fix a leak in setup_tracking
  branch: fix a leak in cmd_branch
  config: fix a leak in git_config_copy_or_rename_section_in_file
  tests: mark as passing with SANITIZE=leak

 branch.c                              | 2 +-
 builtin/branch.c                      | 2 ++
 builtin/rev-parse.c                   | 5 ++++-
 config.c                              | 1 +
 t/t1507-rev-parse-upstream.sh         | 1 +
 t/t1508-at-combinations.sh            | 1 +
 t/t1514-rev-parse-push.sh             | 1 +
 t/t2027-checkout-track.sh             | 1 +
 t/t3200-branch.sh                     | 1 +
 t/t3204-branch-name-interpretation.sh | 1 +
 t/t5404-tracking-branches.sh          | 1 +
 t/t5517-push-mirror.sh                | 1 +
 t/t5525-fetch-tagopt.sh               | 1 +
 t/t6040-tracking-info.sh              | 1 +
 t/t7508-status.sh                     | 1 +
 15 files changed, 19 insertions(+), 2 deletions(-)

To handle "--abbrev-ref" we use shorten_unambiguous_ref(). This function takes a refname and returns a shortened refname, which is a newly allocated string that needs to be freed.

Unfortunately, the refname variable is reused to receive the shortened one. Therefore, we lose the original refname, which needs to be freed as well, producing a leak.

This leak can be reviewed with:

   $ for a in {1..10}; do git branch foo_${a}; done
   $ git rev-parse --abbrev-ref refs/heads/foo_{1..10}

   Direct leak of 171 byte(s) in 10 object(s) allocated from:
       ... in xstrdup wrapper.c
       ... in expand_ref refs.c
       ... in repo_dwim_ref refs.c
       ... in show_rev builtin/rev-parse.c
       ... in cmd_rev_parse builtin/rev-parse.c
       ... in run_builtin git.c

We have this leak since a45d34691e (rev-parse: --abbrev-ref option to shorten ref name, 2009-04-13) when "--abbrev-ref" was introduced.

Let's fix it.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/rev-parse.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

In bdaf1dfae7 (branch: new autosetupmerge option "simple" for matching branches, 2022-04-29) a new exit for setup_tracking() missed the clean-up, producing a leak.

   $ git config branch.autoSetupMerge simple
   $ git remote add local .
   $ git update-ref refs/remotes/local/foo HEAD
   $ git branch bar local/foo

   Direct leak of 384 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in string_list_append_nodup string-list.c
       ... in find_tracked_branch branch.c
       ... in for_each_remote remote.c
       ... in setup_tracking branch.c
       ... in create_branch branch.c
       ... in cmd_branch builtinbranch.c
       ... in run_builtin git.c

   Indirect leak of 24 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in strbuf_grow strbuf.c
       ... in strbuf_add strbuf.c
       ... in match_name_with_pattern remote.c
       ... in query_refspecs remote.c
       ... in remote_find_tracking remote.c
       ... in find_tracked_branch branch.c
       ... in for_each_remote remote.c
       ... in setup_tracking branch.c
       ... in create_branch branch.c
       ... in cmd_branch builtinbranch.c
       ... in run_builtin git.c

The return introduced in bdaf1dfae7 was to avoid setting up the tracking, but even in that case it is still necessary to do the clean-up. Let's do it.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

In 98e7ab6d42 (for-each-ref: delay parsing of --sort=<atom> options, 2021-10-20) a new string_list was introduced to accumulate any "branch.sort" setting.

That string_list is cleared in ref_sorting_options(), which is only called when processing the "--list" sub-command. Therefore, with other sub-command, while having any sort option set, a leak is produced, e.g.:

   $ git config branch.sort invalid_sort_option
   $ git branch --edit-description

   Direct leak of 384 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in string_list_append_nodup string-list.c
       ... in string_list_append string-list.c
       ... in git_branch_config builtin/branch.c
       ... in configset_iter config.c
       ... in repo_config config.c
       ... in git_config config.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

   Indirect leak of 20 byte(s) in 1 object(s) allocated from:
       ... in xstrdup wrapper.c
       ... in string_list_append string-list.c
       ... in git_branch_config builtin/branch.c
       ... in configset_iter config.c
       ... in repo_config config.c
       ... in git_config config.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

We don't have a common clean-up section in cmd_branch(). To avoid refactoring, keep the fix simple, and while we find a better solution which hopefuly will avoid entirely that string_list, when no sort options are needed; let's squelch the leak sanitizer using UNLEAK().

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/branch.c | 2 ++
 1 file changed, 2 insertions(+)

A branch can have its configuration spread over several configuration sections. This situation was already foreseen in 52d59cc645 (branch: add a --copy (-c) option to go with --move (-m), 2017-06-18) when "branch -c" was introduced.

Unfortunately, a leak was also introduced:

   $ git branch foo
   $ cat >> .git/config <<EOF
   [branch "foo"]
   	some-key-a = a value
   [branch "foo"]
   	some-key-b = b value
   [branch "foo"]
   	some-key-c = c value
   EOF
   $ git branch -c foo bar

   Direct leak of 130 byte(s) in 2 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in strbuf_grow strbuf.c
       ... in strbuf_vaddf strbuf.c
       ... in strbuf_addf strbuf.c
       ... in store_create_section config.c
       ... in git_config_copy_or_rename_section_in_file config.c
       ... in git_config_copy_section_in_file config.c
       ... in git_config_copy_section config.c
       ... in copy_or_rename_branch builtin/branch.c
       ... in cmd_branch builtin/branch.c
       ... in run_builtin git.c

Let's fix it.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 config.c | 1 +
 1 file changed, 1 insertion(+)

The tests listed below, since previous commits, no longer trigger any leak.

   + t1507-rev-parse-upstream.sh
   + t1508-at-combinations.sh
   + t1514-rev-parse-push.sh
   + t2027-checkout-track.sh
   + t3200-branch.sh
   + t3204-branch-name-interpretation.sh
   + t5404-tracking-branches.sh
   + t5517-push-mirror.sh
   + t5525-fetch-tagopt.sh
   + t6040-tracking-info.sh
   + t7508-status.sh

Let's mark them with "TEST_PASSES_SANITIZE_LEAK=true" to notice and fix promptly any new leak that may be introduced and triggered by them in the future.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t1507-rev-parse-upstream.sh         | 1 +
 t/t1508-at-combinations.sh            | 1 +
 t/t1514-rev-parse-push.sh             | 1 +
 t/t2027-checkout-track.sh             | 1 +
 t/t3200-branch.sh                     | 1 +
 t/t3204-branch-name-interpretation.sh | 1 +
 t/t5404-tracking-branches.sh          | 1 +
 t/t5517-push-mirror.sh                | 1 +
 t/t5525-fetch-tagopt.sh               | 1 +
 t/t6040-tracking-info.sh              | 1 +
 t/t7508-status.sh                     | 1 +
 11 files changed, 11 insertions(+)