Subject: Re: [PATCH v2 05/24] submodule--helper: "struct pathspec" memory leak in module_update()

From: Junio C Hamano <[email-masked]>
Date: Wed, 20 Jul 2022 09:43:37 -0700

Ævar Arnfjörð Bjarmason  <[email-masked]> writes:

> The module_update() function calls module_list_compute() twice, which
> in turn will reset the "struct pathspec" passed to it. Let's instead
> track two of them, and clear them both.

Looks correct.  I wish there were an easy way to limit the scope of
this new pathspec2 to the block guarded by "if (opt.init)", but with
the "jump to error-exit" idiom, there unfortunately isn't.

Thanks.

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
> ---
>  builtin/submodule--helper.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 28c5fdb8954..7466e781e97 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2602,6 +2602,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 = { 0 };
> +	struct pathspec pathspec2 = { 0 };
>  	struct update_data opt = UPDATE_DATA_INIT;
>  	struct list_objects_filter_options filter_options = { 0 };
>  	int ret;
> @@ -2692,7 +2693,7 @@ 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) {
> +					&pathspec2, &list) < 0) {
>  			ret = 1;
>  			goto cleanup;
>  		}
> @@ -2715,6 +2716,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
>  cleanup:
>  	list_objects_filter_release(&filter_options);
>  	clear_pathspec(&pathspec);
> +	clear_pathspec(&pathspec2);
>  	return ret;
>  }