[09/11] branch: fix a leak in setup_tracking

Commit Message

Rubén Justo June 11, 2023, 6:50 p.m. UTC

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 <[e-mail]>
---
 branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jeff King June 12, 2023, 3:59 a.m. UTC | #1

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

Patch

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: