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

From: Glen Choo <chooglen@google.com>
Date: Jul 13, 2022, 17:37 UTC
Message-ID: <kl6lmtdc6hhp.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <patch-03.11-e5ec6945409-20220713T131601Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> The "path" member can come from "argv" (i.e. not malloc'd), or it can
> be something we determine at runtime. In the latter case let's save
> away a pointer to free() to avoid leaking memory.

[...]

Show 34 quoted lines

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 73717be957c..23ab9c7e349 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1511,6 +1511,7 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
>  struct module_clone_data {
>  	const char *prefix;
>  	const char *path;
> +	char *path_free;
>  	const char *name;
>  	const char *url;
>  	const char *depth;
> @@ -1527,6 +1528,11 @@ struct module_clone_data {
>  	.single_branch = -1, \
>  }
>  
> +static void module_clone_data_release(struct module_clone_data *cd)
> +{
> +	free(cd->path_free);
> +}
> +
>  struct submodule_alternate_setup {
>  	const char *submodule_name;
>  	enum SUBMODULE_ALTERNATE_ERROR_MODE {
> @@ -1651,9 +1657,9 @@ static int clone_submodule(struct module_clone_data *clone_data)
>  
>  	if (!is_absolute_path(clone_data->path)) {
>  		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path);
> -		clone_data->path = strbuf_detach(&sb, NULL);
> +		clone_data->path = clone_data->path_free = strbuf_detach(&sb, NULL);
>  	} else {
> -		clone_data->path = xstrdup(clone_data->path);
> +		clone_data->path = clone_data->path_free = xstrdup(clone_data->path);
>  	}

Hm, having .path_free doesn't seem necessary. If I'm reading the code correctly,

- in module_clone() we set clone_data.path from argv
- then we unconditionally call clone_submodule(), which does all of the
  hard work
- in clone_submodule(), we always hit this conditional, which means that
  past this point, clone_data.path is always free()-able.

which suggests that we don't need clone_data.path_free at all. I suspect that just this

   static void module_clone_data_release(struct module_clone_data *cd)
   {
   	free(cd->path);
   }

is enough to plug the leak (though admittedly, I haven't properly tested the leak because it's been a PITA to set up locally).

That's a pretty hacky suggestion though, since we're still using the same member to hold free()-able and non-free()-able memory. Instead, maybe we could move this "clone_data.path = freeable" logic into module_clone(), like:

  diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
  index 73717be957..d67d4b9647 100644
  --- a/builtin/submodule--helper.c
  +++ b/builtin/submodule--helper.c
  @@ -1649,13 +1649,6 @@ static int clone_submodule(struct module_clone_data *clone_data)
    sm_gitdir = absolute_pathdup(sb.buf);
    strbuf_reset(&sb);

  -	if (!is_absolute_path(clone_data->path)) {
  -		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path);
  -		clone_data->path = strbuf_detach(&sb, NULL);
  -	} else {
  -		clone_data->path = xstrdup(clone_data->path);
  -	}
  -
    if (validate_submodule_git_dir(sm_gitdir, clone_data->name) < 0)
      die(_("refusing to create/use '%s' in another submodule's "
            "git dir"), sm_gitdir);
  @@ -1745,12 +1738,13 @@ static int module_clone(int argc, const char **argv, const char *prefix)
    int dissociate = 0, quiet = 0, progress = 0, require_init = 0;
    struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
    struct list_objects_filter_options filter_options = { 0 };
  +	const char *clone_path;

    struct option module_clone_options[] = {
      OPT_STRING(0, "prefix", &clone_data.prefix,
          N_("path"),
          N_("alternative anchor for relative paths")),
  -		OPT_STRING(0, "path", &clone_data.path,
  +		OPT_STRING(0, "path", &clone_path,
          N_("path"),
          N_("where the new submodule will be cloned to")),
      OPT_STRING(0, "name", &clone_data.name,
  @@ -1795,6 +1789,15 @@ static int module_clone(int argc, const char **argv, const char *prefix)
    clone_data.require_init = !!require_init;
    clone_data.filter_options = &filter_options;

  +	if (!is_absolute_path(clone_path)) {
  +		struct strbuf sb = STRBUF_INIT;
  +		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_path);
  +		clone_data.path = strbuf_detach(&sb, NULL);
  +		strbuf_release(&sb);
  +	} else {
  +		clone_data.path = xstrdup(clone_path);
  +	}
  +
    if (argc || !clone_data.url || !clone_data.path || !*(clone_data.path))
      usage_with_options(git_submodule_helper_usage,
            module_clone_options);