Re: [PATCH 15/26] help: fix leaking `struct cmdnames`
- From
Rubén Justo <rjusto@gmail.com>
- Date
- Nov 10, 2024, 21:46 UTC
- Message-ID
- <c376f2ef-fc43-4815-8e26-f13da3729ed1@gmail.com>
- In-Reply-To
- <2fb012662d6c5720be1fe86973640c7a2d6f5681.1730901926.git.ps@pks.im>
On Wed, Nov 06, 2024 at 04:11:03PM +0100, Patrick Steinhardt wrote:
Show 38 quoted lines
> 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()`.
Show 22 quoted lines
> }
>
> 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.
Show 10 quoted lines
> } 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.
Show 15 quoted lines
> }
>
> 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.
Show 6 quoted lines
> } > > void get_version_info(struct strbuf *buf, int show_build_options) > -- > 2.47.0.229.g8f8d6eee53.dirty >