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

From: Glen Choo <chooglen@google.com>
To: Junio C Hamano <gitster@pobox.com>,
	Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	"Johannes Schindelin" <johannes.schindelin@gmx.de>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH 03/11] submodule--helper: avoid memory leak in `update_submodule()`
Date: Thu, 16 Jun 2022 10:51:02 -0700	[thread overview]
Message-ID: <kl6lsfo44hvd.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <xmqq35g5xmmv.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> 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(+)
>
>
>> 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.

Good eye, I hadn't noticed this.

In this case, we're computing the same thing twice in the function -
first unconditionally, then conditionally. So we can actually just reuse
the first result by doing something like

		next.recursive_prefix = xstrfmt("%s/", update_data->displaypath);

(note that we need to append a "/" at the end)

> 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.

Even better, I recently noticed that .recursive_prefix can probably just
be replaced with "--super-prefix", and that will let us dispatch to
get_submodule_displaypath() without this extra dance here.

I'll flesh that out into a series and send it soon.

> 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 ;-)

cc-ed Ævar, who knows even more than I do :)

git-submodule.sh is definitely going away soon \o/ but I don't think we
have plans to get rid of submodule--helper.c just yet, so I think we
should probably still keep this patch.

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