Re: [PATCH 03/11] submodule--helper: avoid memory leak in `update_submodule()`
- From
Glen Choo <chooglen@google.com>
- Date
- Jun 16, 2022, 17:51 UTC
- Message-ID
- <kl6lsfo44hvd.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <xmqq35g5xmmv.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 28 quoted lines
> "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)
Show 20 quoted lines
> 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.
Show 9 quoted lines
> 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.
Show 6 quoted lines
> > Thanks. > >> next.prefix = NULL; >> oidcpy(&next.oid, null_oid()); >> oidcpy(&next.suboid, null_oid());