Re: [PATCH 03/11] submodule--helper: fix "module_clone_data" memory leaks

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 14, 2022, 00:06 UTC
Message-ID: <220714.86k08g361i.gmgdl@evledraar.gmail.com>
In-Reply-To: <kl6l8row62dg.fsf@chooglen-macbookpro.roam.corp.google.com>

On Wed, Jul 13 2022, Glen Choo wrote:

Show 6 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> [...]
>> What do you think?
>
> Frankly I'm ok with moving things around; I think the code could use
> a little cleaning up :)

Yeah, but this whole part of it is something we'll be throwing away entirely anyway, so I wanted to leave it at just fixing the memory leaks as narrowly as possible for now.

I.e. we invoke "clone" from submodule--helper itself, which we don't need to do if we just invoke it as a function, in which case this whole argv v.s. dynamically generated difference goes away.

Well, we'll have a constant string in some cases, but we'll likely either strdup them all with a strvec, or more likely pass the arguments with custom "options" struct or something.

Show 28 quoted lines

> But yeah, I think my suggestion isn't so great -
> it's a bit weird to keep around an auto variable that exists only to be
> dup-ed to the thing we care about. We can forget about that.
>
> I do think that it's worth avoiding the "sometimes dup, sometimes not"
> pattern if we can, though (of course these are just my non-C instincts
> talking), and we can do that here if we just choose not to assign back
> to .path. Something like:
>
>   struct module_clone_data {
>     const char *prefix;
>   -	const char *path;
>   +	char *path;
>   +	const char *path_argv;
>
>   ...
>
>    	if (!is_absolute_path(clone_data->path)) {
>   -		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path);
>   +		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path_argv);
>   +		clone_data->path = strbuf_detach(&sb, NULL);
>    	} else {
>   -		clone_data->path = xstrdup(clone_data->path);
>   +		clone_data->path = xstrdup(clone_data->path_argv);
>    	}
>
> would be clearer to me since the const pointer never points to something
> that the struct actually owns.

I think that actually makes a lot of sense, I'll probably just change it to that. I'll mull over this again when I get to re-rolling this (depending on future comments), thanks!

> But if the "= .to_free = " idiom is well-understood and accepted to the
> point that we don't need to actively avoid "sometimes dup, sometimes
> not", then we should drop my suggestion and just go with your patch :)

FWIW "git grep ' = to_free = ' finds a fair bit of them, but luckily we usually don't need to play that particular game...