Re: [PATCH 1/3] fix xstrdup leak in parse_short_opt

[Patrick Steinhardt <ps@pks.im>]

On Wed, May 07, 2025 at 02:33:21AM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <502024330056@smail.nju.edu.cn>

This commit is missing an explanation of the problem. When does it
trigger? Which subsystem triggers it? Why didn't we observe the issue
beforehand? And given that the solution seems to be a bit more complex
it should also explain how you're fixing the issue.

Furthermore, the subject of such a commit should be prefixed with the
subsystem in which you're fixing the issue. So in your case,
"parse-options:".

I'd recommend reading through "Documentation/MyFirstContribution.adoc",
which explains all of this.

> diff --git a/parse-options.c b/parse-options.c
> index a9a39ecaef6c..4279dfe4d313 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -638,6 +638,16 @@ static int has_subcommands(const struct option *options)
>  	return 0;
>  }
>  
> +static void set_strdup_fn(struct parse_opt_ctx_t *ctx, const struct option *options) {

Style: the opening brace should be on its own line.

> +	for (; options->type != OPTION_END; options++)
> +		;
> +	if (options->value && options->strdup_fn) {
> +		ctx->unknown_opts = options->value;
> +		ctx->strdup_fn = options->strdup_fn;
> +		return;
> +	}

It's not really clear what this is doing. Is the intent to only change
this this value for the last option? If so, why?

> @@ -981,7 +992,11 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
>  					 *
>  					 * This is leaky, too bad.
>  					 */
> -					ctx->argv[0] = xstrdup(ctx->opt - 1);
> +					if (ctx->unknown_opts && ctx->strdup_fn) {
> +						ctx->argv[0] = ctx->strdup_fn(ctx->unknown_opts, ctx->opt - 1);
> +					} else {
> +						ctx->argv[0] = xstrdup(ctx->opt - 1);
> +					}
>  					*(char *)ctx->argv[0] = '-';
>  					goto unknown;
>  				case PARSE_OPT_NON_OPTION:

Hm. I'm at a loss what we're solving here. All of this really should be
explained in the commit message to give context to the reviewer.

> diff --git a/parse-options.h b/parse-options.h
> index 91c3e3c29b3d..af06a09abb8e 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -388,6 +391,12 @@ static char *parse_options_noop_ignored_value MAYBE_UNUSED;
>  }
>  #define OPT_SUBCOMMAND(l, v, fn)    OPT_SUBCOMMAND_F((l), (v), (fn), 0)
>  
> +#define OPT_UNKNOWN(v, fn) { \
> +	.type = OPTION_END, \
> +	.value = (v), \
> +	.strdup_fn = (fn), \
> +}
> +
>  /*
>   * parse_options() will filter out the processed options and leave the
>   * non-option arguments in argv[]. argv0 is assumed program name and
> @@ -496,6 +505,9 @@ struct parse_opt_ctx_t {
>  	const char *prefix;
>  	const char **alias_groups; /* must be in groups of 3 elements! */
>  	struct parse_opt_cmdmode_list *cmdmode_list;
> +
> +	void *unknown_opts;
> +	parse_opt_strdup_fn *strdup_fn;
>  };
>  
>  void parse_options_start(struct parse_opt_ctx_t *ctx,

Okay. So the intent seems to be that we start storing any options that
we don't understand?

> diff --git a/t/helper/test-free-unknown-options.c b/t/helper/test-free-unknown-options.c
> new file mode 100644
> index 000000000000..9d658115ba8f
> --- /dev/null
> +++ b/t/helper/test-free-unknown-options.c
> @@ -0,0 +1,35 @@
> +#include "git-compat-util.h"
> +#include "parse-options.h"
> +#include "setup.h"
> +#include "strvec.h"
> +
> +static const char *const free_unknown_options_usage[] = {
> +    "test-tool free-unknown-options",
> +    NULL
> +};

Can't we expand `test-tool parse-options` instead of introducing a new
helper?

> +int cmd__free_unknown_options(int argc, const char **argv) {
> +    struct strvec *unknown_opts = xmalloc(sizeof(struct strvec));
> +    strvec_init(unknown_opts);
> +    const char *prefix = setup_git_directory();
> +
> +    bool a, b;
> +	struct option options[] = {
> +		OPT_BOOL('a', "test-a", &a, N_("option a, only for test use")),
> +	OPT_BOOL('b', "test-b", &b, N_("option b, only for test use")),
> +	OPT_UNKNOWN(unknown_opts, (parse_opt_strdup_fn *)&strvec_push),
> +	};
> +
> +    parse_options(argc, argv, prefix, options,
> +	free_unknown_options_usage, PARSE_OPT_KEEP_UNKNOWN_OPT);
> +
> +    printf("a = %s\n", a? "true": "false");
> +    printf("b = %s\n", b? "true": "false");
> +
> +    int i;
> +    for (i = 0; i < unknown_opts->nr; i++) {
> +	printf("free unknown option: %s\n", unknown_opts->v[i]);
> +    }
> +    strvec_clear(unknown_opts);
> +    free(unknown_opts);
> +}
> \ No newline at end of file

Missing newline.

> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 6d62a5b53d95..33fa7828b9f6 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -44,6 +44,7 @@ int cmd__parse_options(int argc, const char **argv);
>  int cmd__parse_options_flags(int argc, const char **argv);
>  int cmd__parse_pathspec_file(int argc, const char** argv);
>  int cmd__parse_subcommand(int argc, const char **argv);
> +int cmd__free_unknown_options(int argc, const char **argv);
>  int cmd__partial_clone(int argc, const char **argv);
>  int cmd__path_utils(int argc, const char **argv);
>  int cmd__path_walk(int argc, const char **argv);
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index ca55ea8228c3..773f54103fd3 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -822,4 +822,18 @@ test_expect_success 'u16 limits range' '
>  	test_grep "value 65536 for option .u16. not in range \[0,65535\]" err
>  '
>  
> +cat >expect <<\EOF
> +a = true
> +b = true
> +free unknown option: -c
> +free unknown option: -d
> +EOF

This should be done inside `test_expect_success` itself.

> +test_expect_success 'free unknown options' '
> +	test-tool free-unknown-options -ac -bd \
> +	>output 2>output.err &&
> +	test_cmp expect output &&
> +	test_must_be_empty output.err
> +'
> +
>  test_done

Okay, so the memory leak seems to happen when handling unknown options
indeed, as I started to expect after the middle of this patch. In any
case, this commit really needs to be polished to have a proper commit
message that sets the stage for the reviewer.

Patrick