diff mbox series

[v2,04/24] submodule--helper: fix most "struct pathspec" memory leaks

Message ID patch-v2-04.24-9fb60485c3e-20220719T204458Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series submodule--helper: fix memory leaks | expand

Commit Message

Ævar Arnfjörð Bjarmason July 19, 2022, 8:46 p.m. UTC 
Call clear_pathspec() at the end of various functions that work with
and allocate a "struct pathspec".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/submodule--helper.c | 115 +++++++++++++++++++++++++-----------
 1 file changed, 81 insertions(+), 34 deletions(-)

Comments

Junio C Hamano July 20, 2022, 4:33 p.m. UTC | #1 
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> @@ -269,7 +269,7 @@ static char *get_up_path(const char *path)
>  static int module_list(int argc, const char **argv, const char *prefix)
>  {
>  	int i;
> -	struct pathspec pathspec;
> +	struct pathspec pathspec = { 0 };
>  	struct module_list list = MODULE_LIST_INIT;
>  
>  	struct option module_list_options[] = {
> @@ -278,6 +278,7 @@ static int module_list(int argc, const char **argv, const char *prefix)
>  			   N_("alternative anchor for relative paths")),
>  		OPT_END()
>  	};
> +	int ret;

Adding such a simple variable near the top, perhaps next to "int i",
would probably make it easier to read, especially because you employ
a good pattern below to let the compiler detect unintended use of
the variable uninitialized, i.e. "ret = 1; goto cleanup;" is placed
at each error-exit codepath, and "ret = 0;" is added immediately
before the "cleanup" label.  If somebody jumps to the cleanup label
without setting "ret", the compiler will catch it.

But you can do better by eliminating the need to check.  Initialize
"ret" to non-zero (i.e. "assume failure"), and have each error-exit
codepath just "goto cleanup".  Keep the "ret = 0;" this patch uses
immediately before the "cleanup" label.

> @@ -287,8 +288,10 @@ static int module_list(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix, module_list_options,
>  			     git_submodule_helper_usage, 0);
>  
> -	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
> -		return 1;
> +	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0) {
> +		ret = 1;
> +		goto cleanup;
> +	}

Which will simplify this hunk.

The same exact pattern repeats throughout this patch, and the above
commits apply to all of the hunks, I think.

Thanks.
Glen Choo July 21, 2022, 5:49 p.m. UTC | #2 
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Call clear_pathspec() at the end of various functions that work with
> and allocate a "struct pathspec".
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/submodule--helper.c | 115 +++++++++++++++++++++++++-----------
>  1 file changed, 81 insertions(+), 34 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index b36919b66c5..28c5fdb8954 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1105,7 +1129,7 @@ static int compute_summary_module_list(struct object_id *head_oid,
>  	struct strvec diff_args = STRVEC_INIT;
>  	struct rev_info rev;
>  	struct module_cb_list list = MODULE_CB_LIST_INIT;
> -	int ret = 0;
> +	int ret;
>  
>  	strvec_push(&diff_args, get_diff_cmd(diff_cmd));
>  	if (info->cached)
> @@ -1145,6 +1169,7 @@ static int compute_summary_module_list(struct object_id *head_oid,
>  	else
>  		run_diff_files(&rev, 0);
>  	prepare_submodule_summary(info, &list);
> +	ret = 0;
>  cleanup:
>  	strvec_clear(&diff_args);
>  	release_revisions(&rev);

This function doesn't use a pathspec at all. Was it changed for
consistency reasons or perhaps an accidental inclusion?

All the other hunks look correct.
diff mbox series

Patch

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b36919b66c5..28c5fdb8954 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -269,7 +269,7 @@  static char *get_up_path(const char *path)
 static int module_list(int argc, const char **argv, const char *prefix)
 {
 	int i;
-	struct pathspec pathspec;
+	struct pathspec pathspec = { 0 };
 	struct module_list list = MODULE_LIST_INIT;
 
 	struct option module_list_options[] = {
@@ -278,6 +278,7 @@  static int module_list(int argc, const char **argv, const char *prefix)
 			   N_("alternative anchor for relative paths")),
 		OPT_END()
 	};
+	int ret;
 
 	const char *const git_submodule_helper_usage[] = {
 		N_("git submodule--helper list [--prefix=<path>] [<path>...]"),
@@ -287,8 +288,10 @@  static int module_list(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_list_options,
 			     git_submodule_helper_usage, 0);
 
-	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
-		return 1;
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 
 	for (i = 0; i < list.nr; i++) {
 		const struct cache_entry *ce = list.entries[i];
@@ -302,7 +305,10 @@  static int module_list(int argc, const char **argv, const char *prefix)
 
 		fprintf(stdout, "%s\n", ce->name);
 	}
-	return 0;
+	ret = 0;
+cleanup:
+	clear_pathspec(&pathspec);
+	return ret;
 }
 
 static void for_each_listed_submodule(const struct module_list *list,
@@ -427,7 +433,7 @@  static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
 static int module_foreach(int argc, const char **argv, const char *prefix)
 {
 	struct foreach_cb info = FOREACH_CB_INIT;
-	struct pathspec pathspec;
+	struct pathspec pathspec = { 0 };
 	struct module_list list = MODULE_LIST_INIT;
 
 	struct option module_foreach_options[] = {
@@ -441,12 +447,15 @@  static int module_foreach(int argc, const char **argv, const char *prefix)
 		N_("git submodule foreach [--quiet] [--recursive] [--] <command>"),
 		NULL
 	};
+	int ret;
 
 	argc = parse_options(argc, argv, prefix, module_foreach_options,
 			     git_submodule_helper_usage, 0);
 
-	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
-		return 1;
+	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 
 	info.argc = argc;
 	info.argv = argv;
@@ -454,7 +463,10 @@  static int module_foreach(int argc, const char **argv, const char *prefix)
 
 	for_each_listed_submodule(&list, runcommand_in_submodule_cb, &info);
 
-	return 0;
+	ret = 0;
+cleanup:
+	clear_pathspec(&pathspec);
+	return ret;
 }
 
 static int starts_with_dot_slash(const char *const path)
@@ -562,7 +574,7 @@  static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data
 static int module_init(int argc, const char **argv, const char *prefix)
 {
 	struct init_cb info = INIT_CB_INIT;
-	struct pathspec pathspec;
+	struct pathspec pathspec = { 0 };
 	struct module_list list = MODULE_LIST_INIT;
 	int quiet = 0;
 
@@ -575,12 +587,15 @@  static int module_init(int argc, const char **argv, const char *prefix)
 		N_("git submodule init [<options>] [<path>]"),
 		NULL
 	};
+	int ret;
 
 	argc = parse_options(argc, argv, prefix, module_init_options,
 			     git_submodule_helper_usage, 0);
 
-	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
-		return 1;
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 
 	/*
 	 * If there are no path args and submodule.active is set then,
@@ -595,7 +610,10 @@  static int module_init(int argc, const char **argv, const char *prefix)
 
 	for_each_listed_submodule(&list, init_submodule_cb, &info);
 
-	return 0;
+	ret = 0;
+cleanup:
+	clear_pathspec(&pathspec);
+	return ret;
 }
 
 struct status_cb {
@@ -740,7 +758,7 @@  static void status_submodule_cb(const struct cache_entry *list_item,
 static int module_status(int argc, const char **argv, const char *prefix)
 {
 	struct status_cb info = STATUS_CB_INIT;
-	struct pathspec pathspec;
+	struct pathspec pathspec = { 0 };
 	struct module_list list = MODULE_LIST_INIT;
 	int quiet = 0;
 
@@ -755,12 +773,15 @@  static int module_status(int argc, const char **argv, const char *prefix)
 		N_("git submodule status [--quiet] [--cached] [--recursive] [<path>...]"),
 		NULL
 	};
+	int ret;
 
 	argc = parse_options(argc, argv, prefix, module_status_options,
 			     git_submodule_helper_usage, 0);
 
-	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
-		return 1;
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 
 	info.prefix = prefix;
 	if (quiet)
@@ -768,7 +789,10 @@  static int module_status(int argc, const char **argv, const char *prefix)
 
 	for_each_listed_submodule(&list, status_submodule_cb, &info);
 
-	return 0;
+	ret = 0;
+cleanup:
+	clear_pathspec(&pathspec);
+	return ret;
 }
 
 static int module_name(int argc, const char **argv, const char *prefix)
@@ -1105,7 +1129,7 @@  static int compute_summary_module_list(struct object_id *head_oid,
 	struct strvec diff_args = STRVEC_INIT;
 	struct rev_info rev;
 	struct module_cb_list list = MODULE_CB_LIST_INIT;
-	int ret = 0;
+	int ret;
 
 	strvec_push(&diff_args, get_diff_cmd(diff_cmd));
 	if (info->cached)
@@ -1145,6 +1169,7 @@  static int compute_summary_module_list(struct object_id *head_oid,
 	else
 		run_diff_files(&rev, 0);
 	prepare_submodule_summary(info, &list);
+	ret = 0;
 cleanup:
 	strvec_clear(&diff_args);
 	release_revisions(&rev);
@@ -1326,10 +1351,11 @@  static void sync_submodule_cb(const struct cache_entry *list_item, void *cb_data
 static int module_sync(int argc, const char **argv, const char *prefix)
 {
 	struct sync_cb info = SYNC_CB_INIT;
-	struct pathspec pathspec;
+	struct pathspec pathspec = { 0 };
 	struct module_list list = MODULE_LIST_INIT;
 	int quiet = 0;
 	int recursive = 0;
+	int ret;
 
 	struct option module_sync_options[] = {
 		OPT__QUIET(&quiet, N_("suppress output of synchronizing submodule url")),
@@ -1346,8 +1372,10 @@  static int module_sync(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_sync_options,
 			     git_submodule_helper_usage, 0);
 
-	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
-		return 1;
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 
 	info.prefix = prefix;
 	if (quiet)
@@ -1357,7 +1385,10 @@  static int module_sync(int argc, const char **argv, const char *prefix)
 
 	for_each_listed_submodule(&list, sync_submodule_cb, &info);
 
-	return 0;
+	ret = 0;
+cleanup:
+	clear_pathspec(&pathspec);
+	return ret;
 }
 
 struct deinit_cb {
@@ -1464,7 +1495,7 @@  static void deinit_submodule_cb(const struct cache_entry *list_item,
 static int module_deinit(int argc, const char **argv, const char *prefix)
 {
 	struct deinit_cb info = DEINIT_CB_INIT;
-	struct pathspec pathspec;
+	struct pathspec pathspec = { 0 };
 	struct module_list list = MODULE_LIST_INIT;
 	int quiet = 0;
 	int force = 0;
@@ -1481,6 +1512,7 @@  static int module_deinit(int argc, const char **argv, const char *prefix)
 		N_("git submodule deinit [--quiet] [-f | --force] [--all | [--] [<path>...]]"),
 		NULL
 	};
+	int ret;
 
 	argc = parse_options(argc, argv, prefix, module_deinit_options,
 			     git_submodule_helper_usage, 0);
@@ -1494,8 +1526,10 @@  static int module_deinit(int argc, const char **argv, const char *prefix)
 	if (!argc && !all)
 		die(_("Use '--all' if you really want to deinitialize all submodules"));
 
-	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
-		return 1;
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 
 	info.prefix = prefix;
 	if (quiet)
@@ -1505,7 +1539,10 @@  static int module_deinit(int argc, const char **argv, const char *prefix)
 
 	for_each_listed_submodule(&list, deinit_submodule_cb, &info);
 
-	return 0;
+	ret = 0;
+cleanup:
+	clear_pathspec(&pathspec);
+	return ret;
 }
 
 struct module_clone_data {
@@ -2564,7 +2601,7 @@  static int update_submodules(struct update_data *update_data)
 
 static int module_update(int argc, const char **argv, const char *prefix)
 {
-	struct pathspec pathspec;
+	struct pathspec pathspec = { 0 };
 	struct update_data opt = UPDATE_DATA_INIT;
 	struct list_objects_filter_options filter_options = { 0 };
 	int ret;
@@ -2643,8 +2680,8 @@  static int module_update(int argc, const char **argv, const char *prefix)
 		opt.update_strategy.type = opt.update_default;
 
 	if (module_list_compute(argc, argv, prefix, &pathspec, &opt.list) < 0) {
-		list_objects_filter_release(&filter_options);
-		return 1;
+		ret = 1;
+		goto cleanup;
 	}
 
 	if (pathspec.nr)
@@ -2655,8 +2692,10 @@  static int module_update(int argc, const char **argv, const char *prefix)
 		struct init_cb info = INIT_CB_INIT;
 
 		if (module_list_compute(argc, argv, opt.prefix,
-					&pathspec, &list) < 0)
-			return 1;
+					&pathspec, &list) < 0) {
+			ret = 1;
+			goto cleanup;
+		}
 
 		/*
 		 * If there are no path args and submodule.active is set then,
@@ -2673,7 +2712,9 @@  static int module_update(int argc, const char **argv, const char *prefix)
 	}
 
 	ret = update_submodules(&opt);
+cleanup:
 	list_objects_filter_release(&filter_options);
+	clear_pathspec(&pathspec);
 	return ret;
 }
 
@@ -2757,9 +2798,10 @@  static int push_check(int argc, const char **argv, const char *prefix)
 static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 {
 	int i;
-	struct pathspec pathspec;
+	struct pathspec pathspec = { 0 };
 	struct module_list list = MODULE_LIST_INIT;
 	unsigned flags = ABSORB_GITDIR_RECURSE_SUBMODULES;
+	int ret;
 
 	struct option embed_gitdir_options[] = {
 		OPT_STRING(0, "prefix", &prefix,
@@ -2778,13 +2820,18 @@  static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, embed_gitdir_options,
 			     git_submodule_helper_usage, 0);
 
-	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
-		return 1;
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 
 	for (i = 0; i < list.nr; i++)
 		absorb_git_dir_into_superproject(list.entries[i]->name, flags);
 
-	return 0;
+	ret = 0;
+cleanup:
+	clear_pathspec(&pathspec);
+	return ret;
 }
 
 static int is_active(int argc, const char **argv, const char *prefix)