Re: [PATCH 15/26] help: fix leaking `struct cmdnames`

["Rubén Justo" <rjusto@gmail.com>]

On Wed, Nov 06, 2024 at 04:11:03PM +0100, Patrick Steinhardt wrote:
> We're populating multiple `struct cmdnames`, but don't ever free them.
> Create a common exit path where we do so.
> 
> Note that this also requires us to use `FREE_AND_NULL()` when freeing
> `cmdnames::names`, as we may otherwise try to free it multiple times.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  help.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/help.c b/help.c
> index 8794f81db9b..51adc530d7a 100644
> --- a/help.c
> +++ b/help.c
> @@ -169,7 +169,7 @@ void cmdnames_release(struct cmdnames *cmds)
>  	int i;
>  	for (i = 0; i < cmds->cnt; ++i)
>  		free(cmds->names[i]);
> -	free(cmds->names);
> +	FREE_AND_NULL(cmds->names);
>  	cmds->cnt = 0;
>  	cmds->alloc = 0;
>  }
> @@ -619,6 +619,7 @@ const char *help_unknown_cmd(const char *cmd)
>  	struct cmdnames main_cmds = { 0 };
>  	struct cmdnames other_cmds = { 0 };
>  	struct cmdname_help *common_cmds;
> +	const char *assumed = NULL;
>  
>  	read_early_config(the_repository, git_unknown_cmd_config, &cfg);
>  
> @@ -630,7 +631,7 @@ const char *help_unknown_cmd(const char *cmd)
>  
>  	if (cfg.autocorrect == AUTOCORRECT_NEVER) {
>  		fprintf_ln(stderr, _("git: '%s' is not a git command. See 'git --help'."), cmd);
> -		exit(1);
> +		goto out;

We haven't set a value for `assumed` at this point, so it's NULL, and in the
new exit path we `exit(1)` when `assumed` is NULL.  OK.

However, I think we don't need this change.  And keeping the `exit(1)` close to
the error message seems like a good idea.  Perhaps, in another series, we could
change it to `die()`.

>  	}
>  
>  	load_command_list("git-", &main_cmds, &other_cmds);
> @@ -695,7 +696,7 @@ const char *help_unknown_cmd(const char *cmd)
>  			; /* still counting */
>  	}
>  	if (cfg.autocorrect && n == 1 && SIMILAR_ENOUGH(best_similarity)) {
> -		const char *assumed = main_cmds.names[0]->name;
> +		assumed = main_cmds.names[0]->name;
>  		main_cmds.names[0] = NULL;
>  		cmdnames_release(&main_cmds);
>  		fprintf_ln(stderr,
> @@ -714,8 +715,10 @@ const char *help_unknown_cmd(const char *cmd)
>  			answer = git_prompt(msg.buf, PROMPT_ECHO);
>  			strbuf_release(&msg);
>  			if (!(starts_with(answer, "y") ||
> -			      starts_with(answer, "Y")))
> -				exit(1);
> +			      starts_with(answer, "Y"))) {
> +				assumed = NULL;
> +				goto out;
> +			}

OK.  But, as above, I don't think we need to change this either.

>  		} else {
>  			fprintf_ln(stderr,
>  				   _("Continuing in %0.1f seconds, "
> @@ -723,7 +726,8 @@ const char *help_unknown_cmd(const char *cmd)
>  				   (float)cfg.autocorrect/10.0, assumed);
>  			sleep_millisec(cfg.autocorrect * 100);
>  		}
> -		return assumed;
> +
> +		goto out;

OK.

>  	}
>  
>  	fprintf_ln(stderr, _("git: '%s' is not a git command. See 'git --help'."), cmd);
> @@ -738,7 +742,13 @@ const char *help_unknown_cmd(const char *cmd)
>  			fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);
>  	}
>  
> -	exit(1);
> +out:
> +	cmdnames_release(&cfg.aliases);
> +	cmdnames_release(&main_cmds);
> +	cmdnames_release(&other_cmds);
> +	if (!assumed)
> +		exit(1);
> +	return assumed;

OK.

>  }
>  
>  void get_version_info(struct strbuf *buf, int show_build_options)
> -- 
> 2.47.0.229.g8f8d6eee53.dirty
>