Re: [PATCH 03/11] submodule--helper: avoid memory leak in `update_submodule()`

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2022, 04:23 UTC
Message-ID: <xmqq35g5xmmv.fsf@gitster.g>
In-Reply-To: <877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 8 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Reported by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/submodule--helper.c | 2 ++
>  1 file changed, 2 insertions(+)

Show 10 quoted lines

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 5c77dfcffee..d7b8004b933 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data)
>  
>  		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
>  								  update_data->prefix);
> +		free(prefixed_path);
> +

This function has two very similar code block that computes prefixed_path depending on the same condition, and one frees the variable correctly while the other one (i.e. this one) forgets to do so, which is irritating to see.

Perhaps the whole "we have update_data structure, in which recursive_prefix, sm_path and prefix members in it; please set the displaypath member based on these values" should become a helper function, e.g.

	static const char *displaypath_from_update_data(struct update_data *u)
	{
		char *pp, *ret;

		if (u->recursive_prefix)
			pp = xstrfmt("%s%s", u->recursive_prefix, u->sm_path);
		else
			pp = xstrdup(u->sm_path);

		ret = get_submodule_displaypath(pp, u->prefix);
		free(pp);
		return ret;
	}

to avoid duplicated computation.

But the whole thing may become moot, as there seems to be a move to get rid of submodule--helper.c altogether?

I'll refrain from touching this patch and instead redirect it to Glen; perhaps removal of submodule--helper.c involves moving the code here to another file or something, in which case it is far easier if I outsource that to somebody who is actually working on the file ;-)

Thanks.

>  		next.prefix = NULL;
>  		oidcpy(&next.oid, null_oid());
>  		oidcpy(&next.suboid, null_oid());