Re: [PATCH v5 03/17] submodule--helper: fix most "struct pathspec" memory leaks

[Glen Choo <chooglen@google.com>]

Æ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 | 74 +++++++++++++++++++++++++------------
>  1 file changed, 51 insertions(+), 23 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d958da7dddc..92d32f2877f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -367,7 +367,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 };

Out of curiousity, does this zero-initialization do anything for us
leaks-wise?

>  	struct module_list list = MODULE_LIST_INIT;
>  	struct option module_foreach_options[] = {
>  		OPT__QUIET(&info.quiet, N_("suppress output of entering each submodule command")),
> @@ -379,12 +379,13 @@ static int module_foreach(int argc, const char **argv, const char *prefix)
>  		N_("git submodule foreach [--quiet] [--recursive] [--] <command>"),
>  		NULL
>  	};
> +	int ret = 1;
>  
>  	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;
> +		goto cleanup;
>  
>  	info.argc = argc;
>  	info.argv = argv;
> @@ -392,7 +393,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)
> @@ -522,7 +526,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;
>  	struct option module_init_options[] = {
> @@ -533,12 +537,13 @@ static int module_init(int argc, const char **argv, const char *prefix)
>  		N_("git submodule init [<options>] [<path>]"),
>  		NULL
>  	};
> +	int ret = 1;
>  
>  	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;
> +		goto cleanup;
>  
>  	/*
>  	 * If there are no path args and submodule.active is set then,
> @@ -553,7 +558,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 {
> @@ -700,7 +708,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;
>  	struct option module_status_options[] = {
> @@ -713,12 +721,13 @@ static int module_status(int argc, const char **argv, const char *prefix)
>  		N_("git submodule status [--quiet] [--cached] [--recursive] [<path>...]"),
>  		NULL
>  	};
> +	int ret = 1;
>  
>  	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;
> +		goto cleanup;
>  
>  	info.prefix = prefix;
>  	if (quiet)
> @@ -726,7 +735,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;
>  }
>  
>  struct module_cb {
> @@ -1265,7 +1277,7 @@ 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;
> @@ -1279,12 +1291,13 @@ static int module_sync(int argc, const char **argv, const char *prefix)
>  		N_("git submodule sync [--quiet] [--recursive] [<path>]"),
>  		NULL
>  	};
> +	int ret = 1;
>  
>  	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;
> +		goto cleanup;
>  
>  	info.prefix = prefix;
>  	if (quiet)
> @@ -1294,7 +1307,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 {
> @@ -1403,7 +1419,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;
> @@ -1418,6 +1434,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 = 1;
>  
>  	argc = parse_options(argc, argv, prefix, module_deinit_options,
>  			     git_submodule_helper_usage, 0);
> @@ -1432,7 +1449,7 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
>  		die(_("Use '--all' if you really want to deinitialize all submodules"));
>  
>  	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
> -		return 1;
> +		goto cleanup;
>  
>  	info.prefix = prefix;
>  	if (quiet)
> @@ -1442,7 +1459,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 {
> @@ -2531,7 +2551,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;
> @@ -2608,8 +2628,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)
> @@ -2620,8 +2640,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,
> @@ -2638,7 +2660,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;
>  }
>  
> @@ -2722,7 +2746,7 @@ 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;
>  	struct option embed_gitdir_options[] = {
> @@ -2737,17 +2761,21 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
>  		N_("git submodule absorbgitdirs [<options>] [<path>...]"),
>  		NULL
>  	};
> +	int ret = 1;
>  
>  	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;
> +		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 module_config(int argc, const char **argv, const char *prefix)
> -- 
> 2.37.1.1233.ge8b09efaedc