Re: [PATCH 7/7] parse-options: don't leak alias help messages

[Jeff King <peff@peff.net>]

On Mon, Mar 08, 2021 at 06:36:20PM +0000, Andrzej Hunt via GitGitGadget wrote:

> preprocess_options() allocates new strings for help messages for
> OPTION_ALIAS. Therefore we also need to clean those help messages up
> when freeing the returned options.

Yep, makes sense.

> The preprocessed options themselves no longer contain any indication
> that a given option is/was an alias: the easiest and fastest way to
> figure it out is to look back at the original options. Alternatively we
> could iterate over the alias_groups list - but that would require nested
> looping and is likely to be a (little) less efficient.

Yeah, this is a bit ugly. We could probably set a bit in the aliased
struct's flags field to indicate that it's an alias, though.

> As far as I can tell, parse_options() is only ever used once per
> command, and the help messages are small - hence this leak has very
> little impact.

We _could_ UNLEAK() it, but I prefer this actual cleanup. We're getting
far enough away from main() that we aren't actually sure that we'll only
be called once per process.

> +static void free_preprocessed_options(const struct option ** preprocessed_options, const struct option *original_options)

A few style nits:

  - omit the space between "**" and preprocessed_options

  - we'd usually break a long line (after the first parameter comma)

I think preprocessed_options shouldn't be const here. After all, our aim
is to free it! I'm also not sure why it's a pointer-to-pointer. If we
were setting it to NULL after freeing, that would be valuable, but we
don't. So all together:

  static void free_preprocessed_options(struct option *preprocessed_options,
                                        const struct option *original_options)

> +{
> +	int i;
> +
> +	if (!*preprocessed_options) {
> +		return;
> +	}

Style: we'd generally omit the curly braces for a single-liner like
this.

Without the double-pointer, we can omit the extra "*" here, too.

> +	for (i = 0; original_options[i].type != OPTION_END; i++) {
> +		if (original_options[i].type == OPTION_ALIAS) {
> +			free((void *)(*preprocessed_options)[i].help);
> +		}
> +	}

OK, so we look through the original options to find ones that became an
alias, and then free them. Makes sense.

Do the indexes always correspond between the original and the
preprocessed arrays? I _think_ so, but preprocess_options() is a little
hard to follow.

If the preprocess code set a flag in the resulting option, though, we
could make it much more obviously correct. And avoid having to pass
original_options at all.

> +	free((void *)*preprocessed_options);

With the interface suggestions above, this becomes just:

  free(preprocessed_options);

> @@ -838,15 +855,17 @@ int parse_options(int argc, const char **argv, const char *prefix,
>  		  int flags)
>  {
>  	struct parse_opt_ctx_t ctx;
> -	struct option *real_options;
> +	const struct option *preprocessed_options, *original_options = NULL;
>  
>  	disallow_abbreviated_options =
>  		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
>  
>  	memset(&ctx, 0, sizeof(ctx));
> -	real_options = preprocess_options(&ctx, options);
> -	if (real_options)
> -		options = real_options;
> +	preprocessed_options = preprocess_options(&ctx, options);
> +	if (preprocessed_options) {
> +		original_options = options;
> +		options = preprocessed_options;
> +	}

OK, we have to keep two variables now rather than aliasing "options",
because we need the original for feeding to the free function (but this
hunk too would go away if we set a flag).

To spell it out, I mean something like on the writing side:

diff --git a/parse-options.c b/parse-options.c
index fbea16eaf5..43431b96b1 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -678,6 +678,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 			newopt[i].short_name = short_name;
 			newopt[i].long_name = long_name;
 			newopt[i].help = strbuf_detach(&help, NULL);
+			newopt[i].flags |= PARSE_OPT_FROM_ALIAS;
 			break;
 		}
 
diff --git a/parse-options.h b/parse-options.h
index ff6506a504..32b0b49a2d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -47,7 +47,8 @@ enum parse_opt_option_flags {
 	PARSE_OPT_SHELL_EVAL = 256,
 	PARSE_OPT_NOCOMPLETE = 512,
 	PARSE_OPT_COMP_ARG = 1024,
-	PARSE_OPT_CMDMODE = 2048
+	PARSE_OPT_CMDMODE = 2048,
+	PARSE_OPT_FROM_ALIAS = 4096,
 };
 
 enum parse_opt_result {

(as an aside, these manual bitfield values are tedious; I wouldn't be
sad to see them converted to "1 << 0", "1 << 1", and so on).

-Peff