diff mbox series

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

Message ID 2fb012662d6c5720be1fe86973640c7a2d6f5681.1730901926.git.ps@pks.im (mailing list archive)
State New, archived
Headers show
Series [01/26] builtin/blame: fix leaking blame entries with `--incremental` | expand

Commit Message

Patrick Steinhardt Nov. 6, 2024, 3:11 p.m. UTC 
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(-)

Comments

Rubén Justo Nov. 10, 2024, 9:46 p.m. UTC | #1 
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
>
Patrick Steinhardt Nov. 11, 2024, 9:09 a.m. UTC | #2 
On Sun, Nov 10, 2024 at 10:46:29PM +0100, Rubén Justo wrote:
> On Wed, Nov 06, 2024 at 04:11:03PM +0100, Patrick Steinhardt wrote:
> > @@ -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()`.

Yeah, good point indeed. I'll adapt this.

Patrick
diff mbox series

Patch

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;
 	}
 
 	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;
+			}
 		} 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;
 	}
 
 	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;
 }
 
 void get_version_info(struct strbuf *buf, int show_build_options)