# tests: mark as passing with SANITIZE=leak

47 messages from 2023-06-11 to 2023-06-17. Participants: Rubén Justo, Jeff King, Junio C Hamano.
Thread: https://gitlist.dev/t/59850

## Rubén Justo, 2023-06-11 18:29

Subject: tests: mark as passing with SANITIZE=leak
Message-ID: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>
URL: https://gitlist.dev/e/9eb931e7-dd18-8ce8-4d4a-cf3d85066189%40gmail.com

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

-- 
2.40.1

```

## Rubén Justo, 2023-06-11 18:49

Subject: [PATCH 01/11] rev-parse: fix a leak with --abbrev-ref
Message-ID: <dc821efb-bf2a-10bb-4547-98d4ba11dbb3@gmail.com>
URL: https://gitlist.dev/e/dc821efb-bf2a-10bb-4547-98d4ba11dbb3%40gmail.com
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

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

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 852e49e340..9fd7095431 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -141,7 +141,7 @@ static void show_rev(int type, const struct object_id *oid, const char *name)
 	if ((symbolic || abbrev_ref) && name) {
 		if (symbolic == SHOW_SYMBOLIC_FULL || abbrev_ref) {
 			struct object_id discard;
-			char *full;
+			char *full, *to_free = NULL;
 
 			switch (repo_dwim_ref(the_repository, name,
 					      strlen(name), &discard, &full,
@@ -156,9 +156,11 @@ static void show_rev(int type, const struct object_id *oid, const char *name)
 				 */
 				break;
 			case 1: /* happy */
-				if (abbrev_ref)
+				if (abbrev_ref) {
+					to_free = full;
 					full = shorten_unambiguous_ref(full,
 						abbrev_ref_strict);
+				}
 				show_with_type(type, full);
 				break;
 			default: /* ambiguous */
@@ -166,6 +168,7 @@ static void show_rev(int type, const struct object_id *oid, const char *name)
 				break;
 			}
 			free(full);
+			free(to_free);
 		} else {
 			show_with_type(type, name);
 		}
-- 
2.40.1

```

## Rubén Justo, 2023-06-11 18:49

Subject: [PATCH 02/11] config: fix a leak in git_config_copy_or_rename_section_in_file
Message-ID: <2ce6f067-9d02-2a10-5eb9-6820e66f0b84@gmail.com>
URL: https://gitlist.dev/e/2ce6f067-9d02-2a10-5eb9-6820e66f0b84%40gmail.com
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

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

diff --git a/config.c b/config.c
index b79baf83e3..39a7d7422c 100644
--- a/config.c
+++ b/config.c
@@ -3879,6 +3879,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
 	free(filename_buf);
 	config_store_data_clear(&store);
 	strbuf_release(&buf);
+	strbuf_release(&copystr);
 	return ret;
 }
 
-- 
2.40.1

```

## Rubén Justo, 2023-06-11 18:49

Subject: [PATCH 03/11] remote: fix a leak in query_matches_negative_refspec
Message-ID: <a9b27053-ff2b-7b67-f2ba-5691f4bda961@gmail.com>
URL: https://gitlist.dev/e/a9b27053-ff2b-7b67-f2ba-5691f4bda961%40gmail.com
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

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

diff --git a/remote.c b/remote.c
index 0764fca0db..1bcd36e358 100644
--- a/remote.c
+++ b/remote.c
@@ -890,7 +890,7 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite
 {
 	int i, matched_negative = 0;
 	int find_src = !query->src;
-	struct string_list reversed = STRING_LIST_INIT_NODUP;
+	struct string_list reversed = STRING_LIST_INIT_DUP;
 	const char *needle = find_src ? query->dst : query->src;
 
 	/*
-- 
2.40.1

```

## Rubén Justo, 2023-06-11 18:49

Subject: [PATCH 04/11] branch: fix a leak in dwim_and_setup_tracking
Message-ID: <a6fec1ab-4006-2896-a2d9-dc0d8fa1c132@gmail.com>
URL: https://gitlist.dev/e/a6fec1ab-4006-2896-a2d9-dc0d8fa1c132%40gmail.com
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

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

diff --git a/branch.c b/branch.c
index ba3914adf5..a7333a4c32 100644
--- a/branch.c
+++ b/branch.c
@@ -638,9 +638,10 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref,
 			     const char *orig_ref, enum branch_track track,
 			     int quiet)
 {
-	char *real_orig_ref;
+	char *real_orig_ref = NULL;
 	dwim_branch_start(r, orig_ref, track, &real_orig_ref, NULL);
 	setup_tracking(new_ref, real_orig_ref, track, quiet);
+	free(real_orig_ref);
 }
 
 /**
-- 
2.40.1

```

## Rubén Justo, 2023-06-11 18:49

Subject: [PATCH 05/11] branch: fix a leak in setup_tracking
Message-ID: <49a4339b-d736-2c30-0c57-194ab33f377c@gmail.com>
URL: https://gitlist.dev/e/49a4339b-d736-2c30-0c57-194ab33f377c%40gmail.com
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

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

diff --git a/branch.c b/branch.c
index a7333a4c32..ff81c2266a 100644
--- a/branch.c
+++ b/branch.c
@@ -333,7 +333,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 		if (!skip_prefix(tracking.srcs->items[0].string,
 				 "refs/heads/", &tracked_branch) ||
 		    strcmp(tracked_branch, new_ref))
-			return;
+			goto cleanup;
 	}
 
 	if (tracking.srcs->nr < 1)
-- 
2.40.1

```

## Rubén Justo, 2023-06-11 18:50

Subject: [PATCH 06/11] branch: fix a leak in cmd_branch
Message-ID: <6cbdf9b9-227a-4665-5725-6a863676e95d@gmail.com>
URL: https://gitlist.dev/e/6cbdf9b9-227a-4665-5725-6a863676e95d%40gmail.com
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

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

diff --git a/builtin/branch.c b/builtin/branch.c
index e6c2655af6..759480fe8d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -709,7 +709,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	enum branch_track track;
 	struct ref_filter filter;
 	static struct ref_sorting *sorting;
-	struct string_list sorting_options = STRING_LIST_INIT_DUP;
+	static struct string_list sorting_options = STRING_LIST_INIT_DUP;
 	struct ref_format format = REF_FORMAT_INIT;
 
 	struct option options[] = {
-- 
2.40.1

```

## Rubén Justo, 2023-06-11 18:50

Subject: [PATCH 07/11] branch: fix a leak in inherit_tracking
Message-ID: <25e68755-7073-9523-dacc-d79e4e10eb39@gmail.com>
URL: https://gitlist.dev/e/25e68755-7073-9523-dacc-d79e4e10eb39%40gmail.com
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

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

diff --git a/branch.c b/branch.c
index ff81c2266a..19d606d360 100644
--- a/branch.c
+++ b/branch.c
@@ -233,7 +233,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
 		return -1;
 	}
 
-	tracking->remote = xstrdup(branch->remote_name);
+	tracking->remote = branch->remote_name;
 	for (i = 0; i < branch->merge_nr; i++)
 		string_list_append(tracking->srcs, branch->merge_name[i]);
 	return 0;
-- 
2.40.1

```

## Rubén Justo, 2023-06-11 18:50

Subject: [PATCH 08/11] branch: fix a leak in check_tracking_branch
Message-ID: <98c68cb4-abbe-0b86-0052-517f47711271@gmail.com>
URL: https://gitlist.dev/e/98c68cb4-abbe-0b86-0052-517f47711271%40gmail.com
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

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

diff --git a/branch.c b/branch.c
index 19d606d360..09b9563ae7 100644
--- a/branch.c
+++ b/branch.c
@@ -480,9 +480,12 @@ static int check_tracking_branch(struct remote *remote, void *cb_data)
 {
 	char *tracking_branch = cb_data;
 	struct refspec_item query;
+	int res;
 	memset(&query, 0, sizeof(struct refspec_item));
 	query.dst = tracking_branch;
-	return !remote_find_tracking(remote, &query);
+	res = !remote_find_tracking(remote, &query);
+	free(query.src);
+	return res;
 }
 
 static int validate_remote_tracking_branch(char *ref)
-- 
2.40.1

```

## Rubén Justo, 2023-06-11 18:50

Subject: [PATCH 09/11] branch: fix a leak in setup_tracking
Message-ID: <471dcc04-d60c-b745-15f0-05fe4e93ff11@gmail.com>
URL: https://gitlist.dev/e/471dcc04-d60c-b745-15f0-05fe4e93ff11%40gmail.com
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

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

diff --git a/branch.c b/branch.c
index 09b9563ae7..d88f50a48a 100644
--- a/branch.c
+++ b/branch.c
@@ -37,7 +37,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)
 	if (!remote_find_tracking(remote, &tracking->spec)) {
 		switch (++tracking->matches) {
 		case 1:
-			string_list_append(tracking->srcs, tracking->spec.src);
+			string_list_append_nodup(tracking->srcs, tracking->spec.src);
 			tracking->remote = remote->name;
 			break;
 		case 2:
-- 
2.40.1

```

## Rubén Justo, 2023-06-11 18:50

Subject: [PATCH 10/11] config: fix a leak in git_config_copy_or_rename_section_in_file
Message-ID: <3ee93720-dce0-8f90-68bf-0242e5731a36@gmail.com>
URL: https://gitlist.dev/e/3ee93720-dce0-8f90-68bf-0242e5731a36%40gmail.com
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

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

diff --git a/config.c b/config.c
index 39a7d7422c..207e4394a3 100644
--- a/config.c
+++ b/config.c
@@ -3833,6 +3833,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
 						output[0] = '\t';
 					}
 				} else {
+					strbuf_release(&copystr);
 					copystr = store_create_section(new_name, &store);
 				}
 			}
-- 
2.40.1

```

## Rubén Justo, 2023-06-11 18:50

Subject: [PATCH 11/11] tests: mark as passing with SANITIZE=leak
Message-ID: <32527a42-ed33-cfd0-60dc-03ccc679ba20@gmail.com>
URL: https://gitlist.dev/e/32527a42-ed33-cfd0-60dc-03ccc679ba20%40gmail.com
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

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

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index cb9ef7e329..b9af6b3ac0 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -5,6 +5,7 @@ test_description='test <branch>@{upstream} syntax'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 87a4286414..e841309d0e 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -4,6 +4,7 @@ test_description='test various @{X} syntax combinations together'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check() {
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index d868a08110..a835a196aa 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -4,6 +4,7 @@ test_description='test <branch>@{push} syntax'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 resolve () {
diff --git a/t/t2027-checkout-track.sh b/t/t2027-checkout-track.sh
index dca35aa3e3..a8bbc60954 100755
--- a/t/t2027-checkout-track.sh
+++ b/t/t2027-checkout-track.sh
@@ -5,6 +5,7 @@ test_description='tests for git branch --track'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 98b6c8ac34..daf1666df7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -8,6 +8,7 @@ test_description='git branch assorted tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 3399344f25..594e3e43e1 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -9,6 +9,7 @@ This script aims to check the behavior of those corner cases.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 expect_branch() {
diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
index cc07889667..51737eeafe 100755
--- a/t/t5404-tracking-branches.sh
+++ b/t/t5404-tracking-branches.sh
@@ -5,6 +5,7 @@ test_description='tracking branch update checks for git push'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh
index a448e169bd..6d4944a728 100755
--- a/t/t5517-push-mirror.sh
+++ b/t/t5517-push-mirror.sh
@@ -5,6 +5,7 @@ test_description='pushing to a mirror repository'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 D=$(pwd)
diff --git a/t/t5525-fetch-tagopt.sh b/t/t5525-fetch-tagopt.sh
index 45815f7378..3a28f1ded5 100755
--- a/t/t5525-fetch-tagopt.sh
+++ b/t/t5525-fetch-tagopt.sh
@@ -2,6 +2,7 @@
 
 test_description='tagopt variable affects "git fetch" and is overridden by commandline.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 setup_clone () {
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index a313849406..7ddbd96e58 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -5,6 +5,7 @@ test_description='remote tracking stats'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 advance () {
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index aed07c5b62..4c1f03e609 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -5,6 +5,7 @@
 
 test_description='git status'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
-- 
2.40.1

```

## Rubén Justo, 2023-06-11 21:23

Subject: Re: tests: mark as passing with SANITIZE=leak
Message-ID: <72cc21d8-1684-2bae-09a1-de647f49c293@gmail.com>
URL: https://gitlist.dev/e/72cc21d8-1684-2bae-09a1-de647f49c293%40gmail.com
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

```
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.
 
> 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(-)
> 
> -- 
> 2.40.1

```

## Jeff King, 2023-06-12 03:12

Subject: Re: [PATCH 01/11] rev-parse: fix a leak with --abbrev-ref
Message-ID: <20230612031239.GA306352@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20230612031239.GA306352%40coredump.intra.peff.net
In-Reply-To: <dc821efb-bf2a-10bb-4547-98d4ba11dbb3@gmail.com>

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

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

Makes sense, but...

> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 852e49e340..9fd7095431 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -141,7 +141,7 @@ static void show_rev(int type, const struct object_id *oid, const char *name)
>  	if ((symbolic || abbrev_ref) && name) {
>  		if (symbolic == SHOW_SYMBOLIC_FULL || abbrev_ref) {
>  			struct object_id discard;
> -			char *full;
> +			char *full, *to_free = NULL;
>  
>  			switch (repo_dwim_ref(the_repository, name,
>  					      strlen(name), &discard, &full,
> @@ -156,9 +156,11 @@ static void show_rev(int type, const struct object_id *oid, const char *name)
>  				 */
>  				break;
>  			case 1: /* happy */
> -				if (abbrev_ref)
> +				if (abbrev_ref) {
> +					to_free = full;
>  					full = shorten_unambiguous_ref(full,
>  						abbrev_ref_strict);
> +				}
>  				show_with_type(type, full);
>  				break;
>  			default: /* ambiguous */
> @@ -166,6 +168,7 @@ static void show_rev(int type, const struct object_id *oid, const char *name)
>  				break;
>  			}
>  			free(full);
> +			free(to_free);
>  		} else {
>  			show_with_type(type, name);
>  		}

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:

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 852e49e340..6dc8548e1f 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -156,9 +156,12 @@ static void show_rev(int type, const struct object_id *oid, const char *name)
 				 */
 				break;
 			case 1: /* happy */
-				if (abbrev_ref)
-					full = shorten_unambiguous_ref(full,
+				if (abbrev_ref) {
+					char *old = full;
+					full = shorten_unambiguous_ref(old,
 						abbrev_ref_strict);
+					free(old);
+				}
 				show_with_type(type, full);
 				break;
 			default: /* ambiguous */

-Peff

```

## Jeff King, 2023-06-12 03:14

Subject: Re: [PATCH 02/11] config: fix a leak in git_config_copy_or_rename_section_in_file
Message-ID: <20230612031425.GB306352@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20230612031425.GB306352%40coredump.intra.peff.net
In-Reply-To: <2ce6f067-9d02-2a10-5eb9-6820e66f0b84@gmail.com>

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

> 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

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

```

## Jeff King, 2023-06-12 03:17

Subject: Re: [PATCH 03/11] remote: fix a leak in query_matches_negative_refspec
Message-ID: <20230612031732.GC306352@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20230612031732.GC306352%40coredump.intra.peff.net
In-Reply-To: <a9b27053-ff2b-7b67-f2ba-5691f4bda961@gmail.com>

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

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

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

-Peff

```

## Jeff King, 2023-06-12 03:21

Subject: Re: [PATCH 04/11] branch: fix a leak in dwim_and_setup_tracking
Message-ID: <20230612032106.GD306352@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20230612032106.GD306352%40coredump.intra.peff.net
In-Reply-To: <a6fec1ab-4006-2896-a2d9-dc0d8fa1c132@gmail.com>

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

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

Yep, looks good. One small comment:

> diff --git a/branch.c b/branch.c
> index ba3914adf5..a7333a4c32 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -638,9 +638,10 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref,
>  			     const char *orig_ref, enum branch_track track,
>  			     int quiet)
>  {
> -	char *real_orig_ref;
> +	char *real_orig_ref = NULL;
>  	dwim_branch_start(r, orig_ref, track, &real_orig_ref, NULL);
>  	setup_tracking(new_ref, real_orig_ref, track, quiet);
> +	free(real_orig_ref);
>  }

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

```

## Jeff King, 2023-06-12 03:26

Subject: Re: [PATCH 05/11] branch: fix a leak in setup_tracking
Message-ID: <20230612032615.GE306352@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20230612032615.GE306352%40coredump.intra.peff.net
In-Reply-To: <49a4339b-d736-2c30-0c57-194ab33f377c@gmail.com>

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

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

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

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  branch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/branch.c b/branch.c
> index a7333a4c32..ff81c2266a 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -333,7 +333,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  		if (!skip_prefix(tracking.srcs->items[0].string,
>  				 "refs/heads/", &tracked_branch) ||
>  		    strcmp(tracked_branch, new_ref))
> -			return;
> +			goto cleanup;
>  	}
>  
>  	if (tracking.srcs->nr < 1)
> -- 
> 2.40.1

```

## Jeff King, 2023-06-12 03:46

Subject: Re: [PATCH 06/11] branch: fix a leak in cmd_branch
Message-ID: <20230612034639.GF306352@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20230612034639.GF306352%40coredump.intra.peff.net
In-Reply-To: <6cbdf9b9-227a-4665-5725-6a863676e95d@gmail.com>

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

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

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:

diff --git a/builtin/branch.c b/builtin/branch.c
index e6c2655af6..075e580d22 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -832,6 +832,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (list)
 		setup_auto_pager("branch", 1);
 
+	UNLEAK(sorting_options);
+
 	if (delete) {
 		if (!argc)
 			die(_("branch name required"));

> diff --git a/builtin/branch.c b/builtin/branch.c
> index e6c2655af6..759480fe8d 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -709,7 +709,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  	enum branch_track track;
>  	struct ref_filter filter;
>  	static struct ref_sorting *sorting;
> -	struct string_list sorting_options = STRING_LIST_INIT_DUP;
> +	static struct string_list sorting_options = STRING_LIST_INIT_DUP;

It's interesting that the ref_sorting pointer is also static. This goes
back to aedcb7dc75 (branch.c: use 'ref-filter' APIs, 2015-09-23). I
wonder if it was trying to accomplish the same thing (back then we added
directly to the list), though back then we did not have any real
leak-detection tools set up.

It should probably move into the "if (list)" block, but that is
orthogonal to your patch.

As for the more complete fix, I think we'd perhaps want something like
this, which would also detect when --sort is used in a non-list mode (as
your example shows, it's a little complicated because we stuff
configured entries into the same list):

diff --git a/builtin/branch.c b/builtin/branch.c
index e6c2655af6..bcd2d9e1d9 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -708,8 +708,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	    recurse_submodules_explicit = 0;
 	enum branch_track track;
 	struct ref_filter filter;
-	static struct ref_sorting *sorting;
 	struct string_list sorting_options = STRING_LIST_INIT_DUP;
+	size_t nr_configured_sorting_options;
 	struct ref_format format = REF_FORMAT_INIT;
 
 	struct option options[] = {
@@ -773,6 +773,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_branch_usage, options);
 
 	git_config(git_branch_config, &sorting_options);
+	nr_configured_sorting_options = sorting_options.nr;
 
 	track = git_branch_track;
 
@@ -832,6 +833,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (list)
 		setup_auto_pager("branch", 1);
 
+	if (!list) {
+		if (sorting_options.nr != nr_configured_sorting_options)
+			die(_("--sort does not make sense when not listing"));
+		string_list_clear(&sorting_options, 0);
+	}
+
 	if (delete) {
 		if (!argc)
 			die(_("branch name required"));
@@ -840,6 +847,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		print_current_branch_name();
 		return 0;
 	} else if (list) {
+		struct ref_sorting *sorting;
 		/*  git branch --list also shows HEAD when it is detached */
 		if ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)
 			filter.kind |= FILTER_REFS_DETACHED_HEAD;

-Peff

```

## Jeff King, 2023-06-12 03:48

Subject: Re: [PATCH 07/11] branch: fix a leak in inherit_tracking
Message-ID: <20230612034831.GG306352@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20230612034831.GG306352%40coredump.intra.peff.net
In-Reply-To: <25e68755-7073-9523-dacc-d79e4e10eb39@gmail.com>

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

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

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

-Peff

```

## Jeff King, 2023-06-12 03:55

Subject: Re: [PATCH 08/11] branch: fix a leak in check_tracking_branch
Message-ID: <20230612035557.GH306352@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20230612035557.GH306352%40coredump.intra.peff.net
In-Reply-To: <98c68cb4-abbe-0b86-0052-517f47711271@gmail.com>

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

> diff --git a/branch.c b/branch.c
> index 19d606d360..09b9563ae7 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -480,9 +480,12 @@ static int check_tracking_branch(struct remote *remote, void *cb_data)
>  {
>  	char *tracking_branch = cb_data;
>  	struct refspec_item query;
> +	int res;
>  	memset(&query, 0, sizeof(struct refspec_item));
>  	query.dst = tracking_branch;
> -	return !remote_find_tracking(remote, &query);
> +	res = !remote_find_tracking(remote, &query);
> +	free(query.src);
> +	return res;
>  }

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

```

## Jeff King, 2023-06-12 03:59

Subject: Re: [PATCH 09/11] branch: fix a leak in setup_tracking
Message-ID: <20230612035948.GI306352@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20230612035948.GI306352%40coredump.intra.peff.net
In-Reply-To: <471dcc04-d60c-b745-15f0-05fe4e93ff11@gmail.com>

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

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

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

```

## Jeff King, 2023-06-12 04:05

Subject: Re: [PATCH 10/11] config: fix a leak in git_config_copy_or_rename_section_in_file
Message-ID: <20230612040518.GJ306352@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20230612040518.GJ306352%40coredump.intra.peff.net
In-Reply-To: <3ee93720-dce0-8f90-68bf-0242e5731a36@gmail.com>

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

> diff --git a/config.c b/config.c
> index 39a7d7422c..207e4394a3 100644
> --- a/config.c
> +++ b/config.c
> @@ -3833,6 +3833,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
>  						output[0] = '\t';
>  					}
>  				} else {
> +					strbuf_release(&copystr);
>  					copystr = store_create_section(new_name, &store);
>  				}
>  			}

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:

diff --git a/config.c b/config.c
index b79baf83e3..f5a7cced7c 100644
--- a/config.c
+++ b/config.c
@@ -3140,37 +3140,36 @@ static int write_error(const char *filename)
 	return 4;
 }
 
-static struct strbuf store_create_section(const char *key,
-					  const struct config_store_data *store)
+static void store_create_section(const char *key,
+				 const struct config_store_data *store,
+				 struct strbuf *sb)
 {
 	const char *dot;
 	size_t i;
-	struct strbuf sb = STRBUF_INIT;
 
 	dot = memchr(key, '.', store->baselen);
 	if (dot) {
-		strbuf_addf(&sb, "[%.*s \"", (int)(dot - key), key);
+		strbuf_addf(sb, "[%.*s \"", (int)(dot - key), key);
 		for (i = dot - key + 1; i < store->baselen; i++) {
 			if (key[i] == '"' || key[i] == '\\')
-				strbuf_addch(&sb, '\\');
-			strbuf_addch(&sb, key[i]);
+				strbuf_addch(sb, '\\');
+			strbuf_addch(sb, key[i]);
 		}
-		strbuf_addstr(&sb, "\"]\n");
+		strbuf_addstr(sb, "\"]\n");
 	} else {
-		strbuf_addch(&sb, '[');
-		strbuf_add(&sb, key, store->baselen);
-		strbuf_addstr(&sb, "]\n");
+		strbuf_addch(sb, '[');
+		strbuf_add(sb, key, store->baselen);
+		strbuf_addstr(sb, "]\n");
 	}
-
-	return sb;
 }
 
 static ssize_t write_section(int fd, const char *key,
 			     const struct config_store_data *store)
 {
-	struct strbuf sb = store_create_section(key, store);
+	struct strbuf sb = STRBUF_INIT;
 	ssize_t ret;
 
+	store_create_section(key, store, &sb);
 	ret = write_in_full(fd, sb.buf, sb.len);
 	strbuf_release(&sb);
 
@@ -3833,7 +3832,9 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
 						output[0] = '\t';
 					}
 				} else {
-					copystr = store_create_section(new_name, &store);
+					strbuf_reset(&copystr);
+					store_create_section(new_name, &store,
+							     &copystr);
 				}
 			}
 			remove = 0;

```

## Jeff King, 2023-06-12 04:06

Subject: Re: tests: mark as passing with SANITIZE=leak
Message-ID: <20230612040651.GK306352@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20230612040651.GK306352%40coredump.intra.peff.net
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

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

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

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

```

## Junio C Hamano, 2023-06-13 19:34

Subject: Re: tests: mark as passing with SANITIZE=leak
Message-ID: <xmqqv8fr40nf.fsf@gitster.g>
URL: https://gitlist.dev/e/xmqqv8fr40nf.fsf%40gitster.g
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

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

> The goal in this series is to pass t3200 with SANITIZE=leak.
>
> As a result of the fixes, other tests also pass.

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.

```

## Rubén Justo, 2023-06-16 22:34

Subject: Re: [PATCH 01/11] rev-parse: fix a leak with --abbrev-ref
Message-ID: <3e124763-944d-e147-88fb-cb989fbd06b7@gmail.com>
URL: https://gitlist.dev/e/3e124763-944d-e147-88fb-cb989fbd06b7%40gmail.com
In-Reply-To: <20230612031239.GA306352@coredump.intra.peff.net>

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

> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 852e49e340..6dc8548e1f 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -156,9 +156,12 @@ static void show_rev(int type, const struct object_id *oid, const char *name)
>  				 */
>  				break;
>  			case 1: /* happy */
> -				if (abbrev_ref)
> -					full = shorten_unambiguous_ref(full,
> +				if (abbrev_ref) {
> +					char *old = full;
> +					full = shorten_unambiguous_ref(old,
>  						abbrev_ref_strict);
> +					free(old);
> +				}
>  				show_with_type(type, full);
>  				break;
>  			default: /* ambiguous */

Simpler.  Nice.

Thanks!

```

## Rubén Justo, 2023-06-16 22:37

Subject: Re: [PATCH 03/11] remote: fix a leak in query_matches_negative_refspec
Message-ID: <3a675cd7-6847-2a23-561e-5baf513d65e0@gmail.com>
URL: https://gitlist.dev/e/3a675cd7-6847-2a23-561e-5baf513d65e0%40gmail.com
In-Reply-To: <20230612031732.GC306352@coredump.intra.peff.net>

```
On 11-jun-2023 23:17:32, Jeff King wrote:
> On Sun, Jun 11, 2023 at 08:49:35PM +0200, Rubén Justo wrote:
> 
> > 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.
> 
> Ah, clearing a NODUP string-list strikes again. :) I concur with your

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

> reasoning here; switching it to DUP is the best fix.

Thank you for reviewing the change.

```

## Rubén Justo, 2023-06-16 22:45

Subject: Re: [PATCH 04/11] branch: fix a leak in dwim_and_setup_tracking
Message-ID: <e75092a4-2629-85a9-1bd2-3193d6719a65@gmail.com>
URL: https://gitlist.dev/e/e75092a4-2629-85a9-1bd2-3193d6719a65%40gmail.com
In-Reply-To: <20230612032106.GD306352@coredump.intra.peff.net>

```
On 11-jun-2023 23:21:06, Jeff King wrote:
> On Sun, Jun 11, 2023 at 08:49:43PM +0200, Rubén Justo wrote:
> 
> > 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.
> 
> Yep, looks good. One small comment:
> 
> > diff --git a/branch.c b/branch.c
> > index ba3914adf5..a7333a4c32 100644
> > --- a/branch.c
> > +++ b/branch.c
> > @@ -638,9 +638,10 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref,
> >  			     const char *orig_ref, enum branch_track track,
> >  			     int quiet)
> >  {
> > -	char *real_orig_ref;
> > +	char *real_orig_ref = NULL;
> >  	dwim_branch_start(r, orig_ref, track, &real_orig_ref, NULL);
> >  	setup_tracking(new_ref, real_orig_ref, track, quiet);
> > +	free(real_orig_ref);
> >  }
> 
> 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.

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.

```

## Rubén Justo, 2023-06-16 22:46

Subject: Re: [PATCH 05/11] branch: fix a leak in setup_tracking
Message-ID: <bb03d449-adb5-5c4d-0a2f-12a26bd48d8c@gmail.com>
URL: https://gitlist.dev/e/bb03d449-adb5-5c4d-0a2f-12a26bd48d8c%40gmail.com
In-Reply-To: <20230612032615.GE306352@coredump.intra.peff.net>

```
On 11-jun-2023 23:26:15, Jeff King wrote:
> On Sun, Jun 11, 2023 at 08:49:51PM +0200, Rubén Justo wrote:
> 
> > 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.
> 
> 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.

Thanks.

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

Perfect.  Thank you.

```

## Rubén Justo, 2023-06-16 22:50

Subject: Re: [PATCH 06/11] branch: fix a leak in cmd_branch
Message-ID: <2ff000c5-5b90-6271-a676-78c5bc93a054@gmail.com>
URL: https://gitlist.dev/e/2ff000c5-5b90-6271-a676-78c5bc93a054%40gmail.com
In-Reply-To: <20230612034639.GF306352@coredump.intra.peff.net>

```
On 11-jun-2023 23:46:39, Jeff King wrote:
> On Sun, Jun 11, 2023 at 08:50:05PM +0200, Rubén Justo wrote:
> 
> > 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.
> 
> Gross. :)

XD

> 
> 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:
> 
> diff --git a/builtin/branch.c b/builtin/branch.c
> index e6c2655af6..075e580d22 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -832,6 +832,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  	if (list)
>  		setup_auto_pager("branch", 1);
>  
> +	UNLEAK(sorting_options);
> +
>  	if (delete) {
>  		if (!argc)
>  			die(_("branch name required"));

OK.  Will do this.

Thank you for reviewing the change with a wide view.

```

## Rubén Justo, 2023-06-16 23:04

Subject: Re: [PATCH 10/11] config: fix a leak in git_config_copy_or_rename_section_in_file
Message-ID: <21325f3e-a502-fdb6-1c67-500bce47e920@gmail.com>
URL: https://gitlist.dev/e/21325f3e-a502-fdb6-1c67-500bce47e920%40gmail.com
In-Reply-To: <20230612040518.GJ306352@coredump.intra.peff.net>

```
On 12-jun-2023 00:05:18, Jeff King wrote:
> On Sun, Jun 11, 2023 at 08:50:45PM +0200, Rubén Justo wrote:
> 
> > diff --git a/config.c b/config.c
> > index 39a7d7422c..207e4394a3 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -3833,6 +3833,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
> >  						output[0] = '\t';
> >  					}
> >  				} else {
> > +					strbuf_release(&copystr);
> >  					copystr = store_create_section(new_name, &store);
> >  				}
> >  			}
> 
> 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:
> 
> diff --git a/config.c b/config.c
> index b79baf83e3..f5a7cced7c 100644
> --- a/config.c
> +++ b/config.c
> @@ -3140,37 +3140,36 @@ static int write_error(const char *filename)
>  	return 4;
>  }
>  
> -static struct strbuf store_create_section(const char *key,
> -					  const struct config_store_data *store)
> +static void store_create_section(const char *key,
> +				 const struct config_store_data *store,
> +				 struct strbuf *sb)
>  {
>  	const char *dot;
>  	size_t i;
> -	struct strbuf sb = STRBUF_INIT;
>  
>  	dot = memchr(key, '.', store->baselen);
>  	if (dot) {
> -		strbuf_addf(&sb, "[%.*s \"", (int)(dot - key), key);
> +		strbuf_addf(sb, "[%.*s \"", (int)(dot - key), key);
>  		for (i = dot - key + 1; i < store->baselen; i++) {
>  			if (key[i] == '"' || key[i] == '\\')
> -				strbuf_addch(&sb, '\\');
> -			strbuf_addch(&sb, key[i]);
> +				strbuf_addch(sb, '\\');
> +			strbuf_addch(sb, key[i]);
>  		}
> -		strbuf_addstr(&sb, "\"]\n");
> +		strbuf_addstr(sb, "\"]\n");
>  	} else {
> -		strbuf_addch(&sb, '[');
> -		strbuf_add(&sb, key, store->baselen);
> -		strbuf_addstr(&sb, "]\n");
> +		strbuf_addch(sb, '[');
> +		strbuf_add(sb, key, store->baselen);
> +		strbuf_addstr(sb, "]\n");
>  	}
> -
> -	return sb;
>  }
>  
>  static ssize_t write_section(int fd, const char *key,
>  			     const struct config_store_data *store)
>  {
> -	struct strbuf sb = store_create_section(key, store);
> +	struct strbuf sb = STRBUF_INIT;
>  	ssize_t ret;
>  
> +	store_create_section(key, store, &sb);
>  	ret = write_in_full(fd, sb.buf, sb.len);
>  	strbuf_release(&sb);
>  
> @@ -3833,7 +3832,9 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
>  						output[0] = '\t';
>  					}
>  				} else {
> -					copystr = store_create_section(new_name, &store);
> +					strbuf_reset(&copystr);
> +					store_create_section(new_name, &store,
> +							     &copystr);
>  				}
>  			}
>  			remove = 0;

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!

```

## Rubén Justo, 2023-06-16 23:14

Subject: Re: tests: mark as passing with SANITIZE=leak
Message-ID: <4e25776e-a490-091f-6f43-7f8f8413bf7b@gmail.com>
URL: https://gitlist.dev/e/4e25776e-a490-091f-6f43-7f8f8413bf7b%40gmail.com
In-Reply-To: <20230612040651.GK306352@coredump.intra.peff.net>

```
On 12/6/23 6:06, Jeff King wrote:
> On Sun, Jun 11, 2023 at 08:29:09PM +0200, Rubén Justo wrote:
> 
>> Each of the commits (except 11/11) fixes a leak.  They have no
>> dependencies on each other.  As a result, they can be reordered.
> 
> 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
> 

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


```

## Rubén Justo, 2023-06-16 23:27

Subject: [PATCH v2 0/5] tests: mark as passing with SANITIZE=leak
Message-ID: <5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com>
URL: https://gitlist.dev/e/5650c4ed-cec0-d11e-4f68-1661b3638786%40gmail.com
In-Reply-To: <9eb931e7-dd18-8ce8-4d4a-cf3d85066189@gmail.com>

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

-- 
2.40.1

```

## Rubén Justo, 2023-06-16 23:34

Subject: [PATCH v2 1/5] rev-parse: fix a leak with --abbrev-ref
Message-ID: <e097e997-9de9-85cc-4e20-22eb32e98765@gmail.com>
URL: https://gitlist.dev/e/e097e997-9de9-85cc-4e20-22eb32e98765%40gmail.com
In-Reply-To: <5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com>

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

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 852e49e340..d2eb239a08 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -156,9 +156,12 @@ static void show_rev(int type, const struct object_id *oid, const char *name)
 				 */
 				break;
 			case 1: /* happy */
-				if (abbrev_ref)
+				if (abbrev_ref) {
+					char *old = full;
 					full = shorten_unambiguous_ref(full,
 						abbrev_ref_strict);
+					free(old);
+				}
 				show_with_type(type, full);
 				break;
 			default: /* ambiguous */
-- 
2.40.1

```

## Rubén Justo, 2023-06-16 23:34

Subject: [PATCH v2 2/5] branch: fix a leak in setup_tracking
Message-ID: <7943ec83-8ef3-4397-0d2e-47f47069de16@gmail.com>
URL: https://gitlist.dev/e/7943ec83-8ef3-4397-0d2e-47f47069de16%40gmail.com
In-Reply-To: <5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com>

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

diff --git a/branch.c b/branch.c
index 427bde896f..d88f50a48a 100644
--- a/branch.c
+++ b/branch.c
@@ -333,7 +333,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 		if (!skip_prefix(tracking.srcs->items[0].string,
 				 "refs/heads/", &tracked_branch) ||
 		    strcmp(tracked_branch, new_ref))
-			return;
+			goto cleanup;
 	}
 
 	if (tracking.srcs->nr < 1)
-- 
2.40.1

```

## Rubén Justo, 2023-06-16 23:34

Subject: [PATCH v2 3/5] branch: fix a leak in cmd_branch
Message-ID: <cfb18a2d-381f-b9ae-865a-26c1669da3b5@gmail.com>
URL: https://gitlist.dev/e/cfb18a2d-381f-b9ae-865a-26c1669da3b5%40gmail.com
In-Reply-To: <5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com>

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

diff --git a/builtin/branch.c b/builtin/branch.c
index e6c2655af6..075e580d22 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -832,6 +832,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (list)
 		setup_auto_pager("branch", 1);
 
+	UNLEAK(sorting_options);
+
 	if (delete) {
 		if (!argc)
 			die(_("branch name required"));
-- 
2.40.1

```

## Rubén Justo, 2023-06-16 23:34

Subject: [PATCH v2 4/5] config: fix a leak in git_config_copy_or_rename_section_in_file
Message-ID: <adaa7fa7-befe-ce59-7391-5a4ddd3bb2b8@gmail.com>
URL: https://gitlist.dev/e/adaa7fa7-befe-ce59-7391-5a4ddd3bb2b8%40gmail.com
In-Reply-To: <5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com>

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

diff --git a/config.c b/config.c
index 39a7d7422c..c5f4b59ef3 100644
--- a/config.c
+++ b/config.c
@@ -3140,37 +3140,36 @@ static int write_error(const char *filename)
 	return 4;
 }
 
-static struct strbuf store_create_section(const char *key,
-					  const struct config_store_data *store)
+static void store_create_section(const char *key,
+				const struct config_store_data *store,
+				struct strbuf *sb)
 {
 	const char *dot;
 	size_t i;
-	struct strbuf sb = STRBUF_INIT;
 
 	dot = memchr(key, '.', store->baselen);
 	if (dot) {
-		strbuf_addf(&sb, "[%.*s \"", (int)(dot - key), key);
+		strbuf_addf(sb, "[%.*s \"", (int)(dot - key), key);
 		for (i = dot - key + 1; i < store->baselen; i++) {
 			if (key[i] == '"' || key[i] == '\\')
-				strbuf_addch(&sb, '\\');
-			strbuf_addch(&sb, key[i]);
+				strbuf_addch(sb, '\\');
+			strbuf_addch(sb, key[i]);
 		}
-		strbuf_addstr(&sb, "\"]\n");
+		strbuf_addstr(sb, "\"]\n");
 	} else {
-		strbuf_addch(&sb, '[');
-		strbuf_add(&sb, key, store->baselen);
-		strbuf_addstr(&sb, "]\n");
+		strbuf_addch(sb, '[');
+		strbuf_add(sb, key, store->baselen);
+		strbuf_addstr(sb, "]\n");
 	}
-
-	return sb;
 }
 
 static ssize_t write_section(int fd, const char *key,
 			     const struct config_store_data *store)
 {
-	struct strbuf sb = store_create_section(key, store);
+	struct strbuf sb = STRBUF_INIT;
 	ssize_t ret;
 
+	store_create_section(key, store, &sb);
 	ret = write_in_full(fd, sb.buf, sb.len);
 	strbuf_release(&sb);
 
@@ -3833,7 +3832,9 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
 						output[0] = '\t';
 					}
 				} else {
-					copystr = store_create_section(new_name, &store);
+					strbuf_reset(&copystr);
+					store_create_section(new_name, &store,
+							     &copystr);
 				}
 			}
 			remove = 0;
-- 
2.40.1


```

## Rubén Justo, 2023-06-16 23:35

Subject: [PATCH v2 5/5] tests: mark as passing with SANITIZE=leak
Message-ID: <66aae5a7-a2a2-2297-4249-d22fd070e91a@gmail.com>
URL: https://gitlist.dev/e/66aae5a7-a2a2-2297-4249-d22fd070e91a%40gmail.com
In-Reply-To: <5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com>

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

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index cb9ef7e329..b9af6b3ac0 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -5,6 +5,7 @@ test_description='test <branch>@{upstream} syntax'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 87a4286414..e841309d0e 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -4,6 +4,7 @@ test_description='test various @{X} syntax combinations together'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check() {
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index d868a08110..a835a196aa 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -4,6 +4,7 @@ test_description='test <branch>@{push} syntax'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 resolve () {
diff --git a/t/t2027-checkout-track.sh b/t/t2027-checkout-track.sh
index dca35aa3e3..a8bbc60954 100755
--- a/t/t2027-checkout-track.sh
+++ b/t/t2027-checkout-track.sh
@@ -5,6 +5,7 @@ test_description='tests for git branch --track'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 98b6c8ac34..daf1666df7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -8,6 +8,7 @@ test_description='git branch assorted tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 3399344f25..594e3e43e1 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -9,6 +9,7 @@ This script aims to check the behavior of those corner cases.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 expect_branch() {
diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
index cc07889667..51737eeafe 100755
--- a/t/t5404-tracking-branches.sh
+++ b/t/t5404-tracking-branches.sh
@@ -5,6 +5,7 @@ test_description='tracking branch update checks for git push'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh
index a448e169bd..6d4944a728 100755
--- a/t/t5517-push-mirror.sh
+++ b/t/t5517-push-mirror.sh
@@ -5,6 +5,7 @@ test_description='pushing to a mirror repository'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 D=$(pwd)
diff --git a/t/t5525-fetch-tagopt.sh b/t/t5525-fetch-tagopt.sh
index 45815f7378..3a28f1ded5 100755
--- a/t/t5525-fetch-tagopt.sh
+++ b/t/t5525-fetch-tagopt.sh
@@ -2,6 +2,7 @@
 
 test_description='tagopt variable affects "git fetch" and is overridden by commandline.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 setup_clone () {
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index a313849406..7ddbd96e58 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -5,6 +5,7 @@ test_description='remote tracking stats'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 advance () {
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index aed07c5b62..4c1f03e609 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -5,6 +5,7 @@
 
 test_description='git status'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
-- 
2.40.1

```

## Junio C Hamano, 2023-06-16 23:45

Subject: Re: [PATCH v2 0/5] tests: mark as passing with SANITIZE=leak
Message-ID: <xmqqfs6rrmxk.fsf@gitster.g>
URL: https://gitlist.dev/e/xmqqfs6rrmxk.fsf%40gitster.g
In-Reply-To: <5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com>

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

> 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!

```

## Jeff King, 2023-06-17 05:48

Subject: Re: [PATCH v2 0/5] tests: mark as passing with SANITIZE=leak
Message-ID: <20230617054823.GG562686@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20230617054823.GG562686%40coredump.intra.peff.net
In-Reply-To: <5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com>

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

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

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

```

## Rubén Justo, 2023-06-17 06:35

Subject: Re: [PATCH v2 0/5] tests: mark as passing with SANITIZE=leak
Message-ID: <5b7cd886-5747-b6df-dcfb-1cc8624d982d@gmail.com>
URL: https://gitlist.dev/e/5b7cd886-5747-b6df-dcfb-1cc8624d982d%40gmail.com
In-Reply-To: <20230617054823.GG562686@coredump.intra.peff.net>

```
On 17/6/23 7:48, Jeff King wrote:
> These all look good to me. I'd probably have split the config.c refactor
> into its own commit

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

Thank you!

```

## Rubén Justo, 2023-06-17 06:37

Subject: [PATCH v3 0/5] tests: mark as passing with SANITIZE=leak
Message-ID: <71b49be1-eeef-2f60-38f1-cfcbe4c8252f@gmail.com>
URL: https://gitlist.dev/e/71b49be1-eeef-2f60-38f1-cfcbe4c8252f%40gmail.com
In-Reply-To: <5650c4ed-cec0-d11e-4f68-1661b3638786@gmail.com>

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

-- 
2.40.1

```

## Rubén Justo, 2023-06-17 06:40

Subject: [PATCH v3 1/5] rev-parse: fix a leak with --abbrev-ref
Message-ID: <38534e6c-f245-38ad-1468-742b5756ab42@gmail.com>
URL: https://gitlist.dev/e/38534e6c-f245-38ad-1468-742b5756ab42%40gmail.com
In-Reply-To: <71b49be1-eeef-2f60-38f1-cfcbe4c8252f@gmail.com>

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

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 852e49e340..d2eb239a08 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -156,9 +156,12 @@ static void show_rev(int type, const struct object_id *oid, const char *name)
 				 */
 				break;
 			case 1: /* happy */
-				if (abbrev_ref)
+				if (abbrev_ref) {
+					char *old = full;
 					full = shorten_unambiguous_ref(full,
 						abbrev_ref_strict);
+					free(old);
+				}
 				show_with_type(type, full);
 				break;
 			default: /* ambiguous */
-- 
2.40.1

```

## Rubén Justo, 2023-06-17 06:41

Subject: [PATCH v3 2/5] branch: fix a leak in setup_tracking
Message-ID: <3d33cd59-d4f3-9112-457f-7d1bfdd08714@gmail.com>
URL: https://gitlist.dev/e/3d33cd59-d4f3-9112-457f-7d1bfdd08714%40gmail.com
In-Reply-To: <71b49be1-eeef-2f60-38f1-cfcbe4c8252f@gmail.com>

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

diff --git a/branch.c b/branch.c
index 427bde896f..d88f50a48a 100644
--- a/branch.c
+++ b/branch.c
@@ -333,7 +333,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 		if (!skip_prefix(tracking.srcs->items[0].string,
 				 "refs/heads/", &tracked_branch) ||
 		    strcmp(tracked_branch, new_ref))
-			return;
+			goto cleanup;
 	}
 
 	if (tracking.srcs->nr < 1)
-- 
2.40.1

```

## Rubén Justo, 2023-06-17 06:41

Subject: [PATCH v3 3/5] branch: fix a leak in cmd_branch
Message-ID: <510e9029-c424-2861-ab3c-30046333f715@gmail.com>
URL: https://gitlist.dev/e/510e9029-c424-2861-ab3c-30046333f715%40gmail.com
In-Reply-To: <71b49be1-eeef-2f60-38f1-cfcbe4c8252f@gmail.com>

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

diff --git a/builtin/branch.c b/builtin/branch.c
index e6c2655af6..075e580d22 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -832,6 +832,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (list)
 		setup_auto_pager("branch", 1);
 
+	UNLEAK(sorting_options);
+
 	if (delete) {
 		if (!argc)
 			die(_("branch name required"));
-- 
2.40.1

```

## Rubén Justo, 2023-06-17 06:41

Subject: [PATCH v3 4/5] config: fix a leak in git_config_copy_or_rename_section_in_file
Message-ID: <33fb45d2-5c6a-b5ad-d405-e7acb8fdbabf@gmail.com>
URL: https://gitlist.dev/e/33fb45d2-5c6a-b5ad-d405-e7acb8fdbabf%40gmail.com
In-Reply-To: <71b49be1-eeef-2f60-38f1-cfcbe4c8252f@gmail.com>

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

diff --git a/config.c b/config.c
index 39a7d7422c..207e4394a3 100644
--- a/config.c
+++ b/config.c
@@ -3833,6 +3833,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
 						output[0] = '\t';
 					}
 				} else {
+					strbuf_release(&copystr);
 					copystr = store_create_section(new_name, &store);
 				}
 			}
-- 
2.40.1

```

## Rubén Justo, 2023-06-17 06:41

Subject: [PATCH v3 5/5] tests: mark as passing with SANITIZE=leak
Message-ID: <94d3610f-4637-5f56-9ab3-7cbd951bc773@gmail.com>
URL: https://gitlist.dev/e/94d3610f-4637-5f56-9ab3-7cbd951bc773%40gmail.com
In-Reply-To: <71b49be1-eeef-2f60-38f1-cfcbe4c8252f@gmail.com>

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

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index cb9ef7e329..b9af6b3ac0 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -5,6 +5,7 @@ test_description='test <branch>@{upstream} syntax'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 87a4286414..e841309d0e 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -4,6 +4,7 @@ test_description='test various @{X} syntax combinations together'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check() {
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index d868a08110..a835a196aa 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -4,6 +4,7 @@ test_description='test <branch>@{push} syntax'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 resolve () {
diff --git a/t/t2027-checkout-track.sh b/t/t2027-checkout-track.sh
index dca35aa3e3..a8bbc60954 100755
--- a/t/t2027-checkout-track.sh
+++ b/t/t2027-checkout-track.sh
@@ -5,6 +5,7 @@ test_description='tests for git branch --track'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 98b6c8ac34..daf1666df7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -8,6 +8,7 @@ test_description='git branch assorted tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 3399344f25..594e3e43e1 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -9,6 +9,7 @@ This script aims to check the behavior of those corner cases.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 expect_branch() {
diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
index cc07889667..51737eeafe 100755
--- a/t/t5404-tracking-branches.sh
+++ b/t/t5404-tracking-branches.sh
@@ -5,6 +5,7 @@ test_description='tracking branch update checks for git push'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh
index a448e169bd..6d4944a728 100755
--- a/t/t5517-push-mirror.sh
+++ b/t/t5517-push-mirror.sh
@@ -5,6 +5,7 @@ test_description='pushing to a mirror repository'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 D=$(pwd)
diff --git a/t/t5525-fetch-tagopt.sh b/t/t5525-fetch-tagopt.sh
index 45815f7378..3a28f1ded5 100755
--- a/t/t5525-fetch-tagopt.sh
+++ b/t/t5525-fetch-tagopt.sh
@@ -2,6 +2,7 @@
 
 test_description='tagopt variable affects "git fetch" and is overridden by commandline.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 setup_clone () {
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index a313849406..7ddbd96e58 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -5,6 +5,7 @@ test_description='remote tracking stats'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 advance () {
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index aed07c5b62..4c1f03e609 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -5,6 +5,7 @@
 
 test_description='git status'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
-- 
2.40.1

```