Re: [PATCH 07/11] submodule--helper: fix "errmsg_str" memory leak

From: Glen Choo <chooglen@google.com>
Date: Jul 14, 2022, 18:20 UTC
Message-ID: <kl6lwncf4kvd.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <patch-07.11-f8c20bbf266-20220713T131601Z-avarab@gmail.com>

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

Show 58 quoted lines

> Fix a memory leak introduced in e83e3333b57 (submodule: port submodule
> subcommand 'summary' from shell to C, 2020-08-13), to do that stop
> juggling around the "errmsg" and "struct strbuf", let's instead move
> the "struct strbuf errmsg" to the top-level.
>
> Now we don't need to strbuf_detach() it anymore, but we do need to
> ensure that we pass NULL to print_submodule_summary() when we have no
> error message.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/submodule--helper.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a964dbeec38..a05578a7382 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -932,7 +932,8 @@ static void generate_submodule_summary(struct summary_cb *info,
>  {
>  	char *displaypath, *src_abbrev = NULL, *dst_abbrev;
>  	int missing_src = 0, missing_dst = 0;
> -	char *errmsg = NULL;
> +	char *errmsg;
> +	struct strbuf errmsg_str = STRBUF_INIT;
>  	int total_commits = -1;
>  
>  	if (!info->cached && oideq(&p->oid_dst, null_oid())) {
> @@ -1032,7 +1033,6 @@ static void generate_submodule_summary(struct summary_cb *info,
>  		 * submodule, i.e., deleted or changed to blob
>  		 */
>  		if (S_ISGITLINK(p->mod_dst)) {
> -			struct strbuf errmsg_str = STRBUF_INIT;
>  			if (missing_src && missing_dst) {
>  				strbuf_addf(&errmsg_str, "  Warn: %s doesn't contain commits %s and %s\n",
>  					    displaypath, oid_to_hex(&p->oid_src),
> @@ -1043,10 +1043,10 @@ static void generate_submodule_summary(struct summary_cb *info,
>  					    oid_to_hex(&p->oid_src) :
>  					    oid_to_hex(&p->oid_dst));
>  			}
> -			errmsg = strbuf_detach(&errmsg_str, NULL);
>  		}
>  	}
>  
> +	errmsg = errmsg_str.len ? errmsg_str.buf : NULL;
>  	print_submodule_summary(info, errmsg, total_commits,
>  				displaypath, src_abbrev,
>  				dst_abbrev, p);
> @@ -1054,6 +1054,7 @@ static void generate_submodule_summary(struct summary_cb *info,
>  	free(displaypath);
>  	free(src_abbrev);
>  	free(dst_abbrev);
> +	strbuf_release(&errmsg_str);
>  }
>  
>  static void prepare_submodule_summary(struct summary_cb *info,
> -- 
> 2.37.0.932.g7b7031e73bc

What do you think of getting rid of "char *errmsg" altogether? We can replace it with errmsg_str.buf and do the length check in print_submodule_summary():

  diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
  index a964dbeec3..f9f0de7e83 100644
  --- a/builtin/submodule--helper.c
  +++ b/builtin/submodule--helper.c
  @@ -896,7 +896,7 @@ static void print_submodule_summary(struct summary_cb *info, char *errmsg,
    else
      printf(" (%d):\n", total_commits);

  -	if (errmsg) {
  +	if (errmsg && *errmsg) {
      printf(_("%s"), errmsg);
    } else if (total_commits > 0) {
      struct child_process cp_log = CHILD_PROCESS_INIT;
  @@ -932,7 +932,7 @@ static void generate_submodule_summary(struct summary_cb *info,
  {
    char *displaypath, *src_abbrev = NULL, *dst_abbrev;
    int missing_src = 0, missing_dst = 0;
  -	char *errmsg = NULL;
  +	struct strbuf errmsg_str = STRBUF_INIT;
    int total_commits = -1;

    if (!info->cached && oideq(&p->oid_dst, null_oid())) {
  @@ -1032,7 +1032,6 @@ static void generate_submodule_summary(struct summary_cb *info,
      * submodule, i.e., deleted or changed to blob
      */
      if (S_ISGITLINK(p->mod_dst)) {
  -			struct strbuf errmsg_str = STRBUF_INIT;
        if (missing_src && missing_dst) {
          strbuf_addf(&errmsg_str, "  Warn: %s doesn't contain commits %s and %s\n",
                displaypath, oid_to_hex(&p->oid_src),
  @@ -1043,17 +1042,17 @@ static void generate_submodule_summary(struct summary_cb *info,
                oid_to_hex(&p->oid_src) :
                oid_to_hex(&p->oid_dst));
        }
  -			errmsg = strbuf_detach(&errmsg_str, NULL);
      }
    }

  -	print_submodule_summary(info, errmsg, total_commits,
  +	print_submodule_summary(info, errmsg_str.buf, total_commits,
          displaypath, src_abbrev,
          dst_abbrev, p);

    free(displaypath);
    free(src_abbrev);
    free(dst_abbrev);
  +	strbuf_release(&errmsg_str);
  }

  static void prepare_submodule_summary(struct summary_cb *info,