Re: [PATCH 08/23] builtin/ls-remote: fix leaking `pattern` strings

[Taylor Blau <me@ttaylorr.com>]

On Fri, Jul 26, 2024 at 02:15:24PM +0200, Patrick Steinhardt wrote:
> Users can pass patterns to git-ls-remote(1), which allows them to filter
> the list of printed references. We assemble those patterns into an array
> and prefix them with "*/", but never free either the array nor the
> allocated strings.
>
> Fix those leaks.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/ls-remote.c          | 11 +++++++----
>  t/t5535-fetch-push-symref.sh |  1 +
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index debf2d4f88..500f76fe4c 100644
> --- a/builtin/ls-remote.c
> +++ b/builtin/ls-remote.c
> @@ -47,7 +47,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  	int status = 0;
>  	int show_symref_target = 0;
>  	const char *uploadpack = NULL;
> -	const char **pattern = NULL;
> +	char **pattern = NULL;
>  	struct transport_ls_refs_options transport_options =
>  		TRANSPORT_LS_REFS_OPTIONS_INIT;
>  	int i;
> @@ -96,9 +96,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  	if (argc > 1) {
>  		int i;
>  		CALLOC_ARRAY(pattern, argc);
> -		for (i = 1; i < argc; i++) {
> +		for (i = 1; i < argc; i++)
>  			pattern[i - 1] = xstrfmt("*/%s", argv[i]);
> -		}

Instead of xstrfmt()-ing these strings directly, it may be more
ergonomic to use a strvec here and call strvec_pushf(). That would save
you explicitly CALLOC'ing the array, and would also save you the
explicit free() on each element of pattern below.

>  	}
>
>  	if (flags & REF_TAGS)
> @@ -136,7 +135,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  		struct ref_array_item *item;
>  		if (!check_ref_type(ref, flags))
>  			continue;
> -		if (!tail_match(pattern, ref->name))
> +		if (!tail_match((const char **) pattern, ref->name))

You could also drop the const cast here as well. The resulting patch on
top of this one simplifies things a bit:

--- 8< ---
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 500f76fe4c..b59ac98d81 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -47,7 +47,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int status = 0;
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
-	char **pattern = NULL;
+	struct strvec pattern = STRVEC_INIT;
 	struct transport_ls_refs_options transport_options =
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
 	int i;
@@ -93,12 +93,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)

 	packet_trace_identity("ls-remote");

-	if (argc > 1) {
-		int i;
-		CALLOC_ARRAY(pattern, argc);
-		for (i = 1; i < argc; i++)
-			pattern[i - 1] = xstrfmt("*/%s", argv[i]);
-	}
+	for (int i = 1; i < argc; i++)
+		strvec_pushf(&pattern, "*/%s", argv[i]);

 	if (flags & REF_TAGS)
 		strvec_push(&transport_options.ref_prefixes, "refs/tags/");
@@ -135,7 +131,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		struct ref_array_item *item;
 		if (!check_ref_type(ref, flags))
 			continue;
-		if (!tail_match((const char **) pattern, ref->name))
+		if (!tail_match(pattern.v, ref->name))
 			continue;
 		item = ref_array_push(&ref_array, ref->name, &ref->old_oid);
 		item->symref = xstrdup_or_null(ref->symref);
@@ -158,8 +154,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		status = 1;
 	transport_ls_refs_options_release(&transport_options);

-	for (i = 1; i < argc; i++)
-		free(pattern[i - 1]);
-	free(pattern);
+	strvec_clear(&pattern);
 	return status;
 }
--- >8 ---

Thanks,
Taylor