From: Andrzej Hunt <andrzej@ahunt.org>
Date: Sun, 14 Mar 2021 17:03:22 GMT
Subject: Re: [PATCH 7/7] parse-options: don't leak alias help messages
Message-ID: <6ddb4ca3-8486-a8cf-24ed-1ae6220602f3@ahunt.org>
In-Reply-To: <YEZ/BWWbpfVwl6nO@coredump.intra.peff.net>

On 08/03/2021 20:46, Jeff King wrote:
> On Mon, Mar 08, 2021 at 06:36:20PM +0000, Andrzej Hunt via GitGitGadget wrote:
> 
>> +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)

I'm not sure what I was originally thinking when I used the 
pointer-to-pointer - I've incorporated your suggestions, they do make 
everything easier to read. Moreover we'll remove original_options as per 
your later suggestions anyway.

> 
>> +	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.

_At this time_, indexes always correspond between the original and 
preprocessed options, but in the back of my mind I was still a little 
bit uncomfortable depending on that. Your suggestion is much better - so 
I've gone ahead and implemented it.

> 
>> +	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).

Indeed - after adding the flag as suggested, the changes to 
parse_options() are reduced down to calling free_preprocessed_options() 
instead of free() - which is quite a nice simplification.

> 
> 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).

I've added a separate patch to take care of the bitfield improvements - 
and have incorporated the PARSE_OPT_FROM_ALIAS change into the original 
patch!