[06/11] branch: fix a leak in cmd_branch

Commit Message

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

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

Comments

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

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

Rubén Justo June 16, 2023, 10:50 p.m. UTC | #2

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.

Patch

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[] = {