Re: [PATCH 7/7] parse-options: don't leak alias help messages
- From
Jeff King <peff@peff.net>
- Date
- Mar 8, 2021, 19:46 UTC
- Message-ID
- <YEZ/BWWbpfVwl6nO@coredump.intra.peff.net>
- In-Reply-To
- <fb456bee0f69e0ca5e596b30705c42cc037edecc.1615228580.git.gitgitgadget@gmail.com>
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.
Show 5 quoted lines
> 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)Show 6 quoted lines
> +{
> + 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.
Show 5 quoted lines
> + 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);
Show 19 quoted lines
> @@ -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