fix xstrdup leak in parse_short_opt

Pass a usered defined strdup-like function a parse_opt_ctx to avoid memory leak.

Lidong Yan (3):
fix xstrdup leak in parse_short_opt
fix: replace bug where int was incorrectly used as bool
fix: use strvec_push_wrapper to prevent ubsan failure

 Makefile                             |  1 +
 parse-options.c                      | 23 ++++++++++++++-
 parse-options.h                      | 12 ++++++++
 t/helper/meson.build                 |  1 +
 t/helper/test-free-unknown-options.c | 42 ++++++++++++++++++++++++++++
 t/helper/test-tool.c                 |  1 +
 t/helper/test-tool.h                 |  1 +
 t/t0040-parse-options.sh             | 14 ++++++++++
 8 files changed, 94 insertions(+), 1 deletion(-)
 create mode 100644 t/helper/test-free-unknown-options.c

base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1954%2Fbrandb97%2Ffix-parse-option-leak-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1954/brandb97/fix-parse-option-leak-v1
Pull-Request: https://github.com/git/git/pull/1954

gitgitgadget

From: Lidong Yan <[email protected]>

---
 Makefile                             |  1 +
 parse-options.c                      | 17 +++++++++++++-
 parse-options.h                      | 12 ++++++++++
 t/helper/meson.build                 |  1 +
 t/helper/test-free-unknown-options.c | 35 ++++++++++++++++++++++++++++
 t/helper/test-tool.c                 |  1 +
 t/helper/test-tool.h                 |  1 +
 t/t0040-parse-options.sh             | 14 +++++++++++
 8 files changed, 81 insertions(+), 1 deletion(-)
 create mode 100644 t/helper/test-free-unknown-options.c

index 8a7f1c76543f..af8ea677b820 100644
@@ -822,6 +822,7 @@ TEST_BUILTINS_OBJS += test-online-cpus.o
 TEST_BUILTINS_OBJS += test-pack-mtimes.o
 TEST_BUILTINS_OBJS += test-parse-options.o
 TEST_BUILTINS_OBJS += test-parse-pathspec-file.o
+TEST_BUILTINS_OBJS += test-free-unknown-options.o
 TEST_BUILTINS_OBJS += test-partial-clone.o
 TEST_BUILTINS_OBJS += test-path-utils.o
 TEST_BUILTINS_OBJS += test-path-walk.o

index a9a39ecaef6c..4279dfe4d313 100644
@@ -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) {
+	for (; options->type != OPTION_END; options++)
+		;
+	if (options->value && options->strdup_fn) {
+		ctx->unknown_opts = options->value;
+		ctx->strdup_fn = options->strdup_fn;
+		return;
+	}
+}
+
 static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
 				  int argc, const char **argv, const char *prefix,
 				  const struct option *options,

@@ -655,6 +665,7 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
 	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
 	ctx->flags = flags;
 	ctx->has_subcommands = has_subcommands(options);
+	set_strdup_fn(ctx, options);
 	if (!ctx->has_subcommands && (flags & PARSE_OPT_SUBCOMMAND_OPTIONAL))
 		BUG("Using PARSE_OPT_SUBCOMMAND_OPTIONAL without subcommands");
 	if (ctx->has_subcommands) {

@@ -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:

index 91c3e3c29b3d..af06a09abb8e 100644
@@ -77,6 +77,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
 typedef int parse_opt_subcommand_fn(int argc, const char **argv,
 				    const char *prefix, struct repository *repo);
 
+typedef char *parse_opt_strdup_fn(void *value, const char *s);
+
 /*
  * `type`::
  *   holds the type of the option, you must have an OPTION_END last in your

@@ -165,6 +167,7 @@ struct option {
 	parse_opt_ll_cb *ll_callback;
 	intptr_t extra;
 	parse_opt_subcommand_fn *subcommand_fn;
+	parse_opt_strdup_fn *strdup_fn;
 };
 
 #define OPT_BIT_F(s, l, v, h, b, f) { \

@@ -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,

index d2cabaa2bcfc..476e32781761 100644
@@ -39,6 +39,7 @@ test_tool_sources = [
   'test-pack-mtimes.c',
   'test-parse-options.c',
   'test-parse-pathspec-file.c',
+  'test-free-unknown-options.c',
   'test-partial-clone.c',
   'test-path-utils.c',
   'test-path-walk.c',

new file mode 100644
index 000000000000..9d658115ba8f
@@ -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
+};
+
+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

index 50dc4dac4ed6..79ec4f9cda07 100644
@@ -51,6 +51,7 @@ static struct test_cmd cmds[] = {
 	{ "parse-options-flags", cmd__parse_options_flags },
 	{ "parse-pathspec-file", cmd__parse_pathspec_file },
 	{ "parse-subcommand", cmd__parse_subcommand },
+	{ "free-unknown-options", cmd__free_unknown_options},
 	{ "partial-clone", cmd__partial_clone },
 	{ "path-utils", cmd__path_utils },
 	{ "path-walk", cmd__path_walk },

index 6d62a5b53d95..33fa7828b9f6 100644
@@ -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);

index ca55ea8228c3..773f54103fd3 100755
@@ -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
+
+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

gitgitgadget

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.

From: Lidong Yan <[email protected]>

---
 parse-options.c                      | 12 ++++++--
 parse-options.h                      | 12 ++++----
 t/helper/test-free-unknown-options.c | 43 ++++++++++++++--------------
 t/helper/test-tool.c                 |  2 +-
 4 files changed, 38 insertions(+), 31 deletions(-)

index 4279dfe4d313..12721b83000a 100644
@@ -638,7 +638,9 @@ 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) {
+static void set_strdup_fn(struct parse_opt_ctx_t *ctx,
+			  const struct option *options)
+{
 	for (; options->type != OPTION_END; options++)
 		;
 	if (options->value && options->strdup_fn) {

@@ -993,9 +995,13 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 					 * This is leaky, too bad.
 					 */
 					if (ctx->unknown_opts && ctx->strdup_fn) {
-						ctx->argv[0] = ctx->strdup_fn(ctx->unknown_opts, ctx->opt - 1);
+						ctx->argv[0] =
+							ctx->strdup_fn(ctx->unknown_opts,
+								       ctx->opt -
+									       1);
 					} else {
-						ctx->argv[0] = xstrdup(ctx->opt - 1);
+						ctx->argv[0] =
+							xstrdup(ctx->opt - 1);
 					}
 					*(char *)ctx->argv[0] = '-';
 					goto unknown;

index af06a09abb8e..57037bd381a3 100644
@@ -391,11 +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), \
-}
+#define OPT_UNKNOWN(v, fn)          \
+	{                           \
+		.type = OPTION_END, \
+		.value = (v),       \
+		.strdup_fn = (fn),  \
+	}
 
 /*
  * parse_options() will filter out the processed options and leave the

@@ -505,7 +506,6 @@ 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;
 };

index 79ec4f9cda07..5af5acae1cb3 100644
@@ -51,7 +51,7 @@ static struct test_cmd cmds[] = {
 	{ "parse-options-flags", cmd__parse_options_flags },
 	{ "parse-pathspec-file", cmd__parse_pathspec_file },
 	{ "parse-subcommand", cmd__parse_subcommand },
-	{ "free-unknown-options", cmd__free_unknown_options},
+	{ "free-unknown-options", cmd__free_unknown_options },
 	{ "partial-clone", cmd__partial_clone },
 	{ "path-utils", cmd__path_utils },
 	{ "path-walk", cmd__path_walk },

gitgitgadget

The same is true here -- this needs to have a proper commit subject and message.

From: Lidong Yan <[email protected]>

---
 t/helper/test-free-unknown-options.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

index 59d732da23ca..7369dfe379d6 100644
@@ -8,6 +8,12 @@ static const char *const free_unknown_options_usage[] = {
 	"test-tool free-unknown-options", NULL
 };
 
+static char *strvec_push_wrapper(void *value, const char *str)
+{
+	struct strvec *sv = value;
+	return (char *)strvec_push(sv, str);
+}
+
 int cmd__free_unknown_options(int argc, const char **argv)
 {
 	struct strvec *unknown_opts = xmalloc(sizeof(struct strvec));

@@ -17,7 +23,7 @@ int cmd__free_unknown_options(int argc, const char **argv)
 	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),
+		OPT_UNKNOWN(unknown_opts, strvec_push_wrapper),
 	};
 
 	strvec_init(unknown_opts);

gitgitgadget

This looks like another fixup-style commit. I assume that all of this really should be a single commit, as the latter two commits don't seem to do anything new compared to the first commit.

Patrick

Thank you for your suggestion. I will squash all the commits and include a detailed description in the next submission.

In short, calling parse_options may cause a memory leak when encountering unknown short options. To fix the leak, parse_options can use a user-provided allocation function, so that the user can later free the memory and avoid the leak. I also defined a macro called OPT_UNKNOWN to make it easier for users to pass in their own allocation function.

On the other hand, I think a new file should be added under t/helper/. The reason is that this new file not only serves as a test, but also provides a simple example of using OPT_UNKNOWN, which others can refer to when modifying their own option definitions to avoid memory leaks.

From: Lidong Yan <[email protected]>

Most Git commands use parse_options to parse options, but parse_options causes a memory leak when it encounters unknown short options. The leak occurs at line 984 in parse-options.c, where git allocates some spaces to store the unknown short options and will never release those spaces.

To address this issue, users can be allowed to provide a custom function to allocate memory for unknown options. For example, users can use strvec_push to allocate memory for unknown options and later call strvec_clear at the end of the Git command to release the memory, thereby avoiding the memory leak.

This commit allows users to provide a custom allocation function for unknown options via the strdup_fn field in the last struct option. For convenience, this commit also introduces a new macro, OPT_UNKNOWN, which behaves like OPT_END but takes an additional argument for the allocation function. parse_options could get the custom allocation function in struct parse_opt_ctx_t by using set_strdup_fn. A simple example to use OPT_UNKNOWN is put into t/helper/test-free-unknown-options.c

---
fix xstrdup leak in parse_short_opt

Pass a user defined strdup-like function in parse_opt_ctx to avoid memory leak.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1954%2Fbrandb97%2Ffix-parse-option-leak-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1954/brandb97/fix-parse-option-leak-v2
Pull-Request: https://github.com/git/git/pull/1954

Range-diff vs v1:

1: b34445d166c ! 1: e7b4465b83e fix xstrdup leak in parse_short_opt

     @@ Metadata
      Author: Lidong Yan <[email protected]>
      
       ## Commit message ##
     -    fix xstrdup leak in parse_short_opt
     +    parse-options: fix xstrdup leak in parse_options_step parse-options:984
     +
     +    Most Git commands use parse_options to parse options, but parse_options
     +    causes a memory leak when it encounters unknown short options. The leak
     +    occurs at line 984 in parse-options.c, where git allocates some spaces
     +    to store the unknown short options and will never release those spaces.
     +
     +    To address this issue, users can be allowed to provide a custom function
     +    to allocate memory for unknown options. For example, users can use
     +    strvec_push to allocate memory for unknown options and later call
     +    strvec_clear at the end of the Git command to release the memory, thereby
     +    avoiding the memory leak.
     +
     +    This commit allows users to provide a custom allocation function for
     +    unknown options via the strdup_fn field in the last struct option. For
     +    convenience, this commit also introduces a new macro, OPT_UNKNOWN, which
     +    behaves like OPT_END but takes an additional argument for the allocation
     +    function. parse_options could get the custom allocation function in struct
     +    parse_opt_ctx_t by using set_strdup_fn. A simple example to use
     +    OPT_UNKNOWN is put into t/helper/test-free-unknown-options.c
      
          Signed-off-by: Lidong Yan <[email protected]>
      
     @@ parse-options.c: 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) {
     ++static void set_strdup_fn(struct parse_opt_ctx_t *ctx,
     ++			  const struct option *options)
     ++{
      +	for (; options->type != OPTION_END; options++)
      +		;
      +	if (options->value && options->strdup_fn) {
     @@ parse-options.c: enum parse_opt_result parse_options_step(struct parse_opt_ctx_t
       					 */
      -					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);
     ++						ctx->argv[0] =
     ++							ctx->strdup_fn(ctx->unknown_opts,
     ++								       ctx->opt -
     ++									       1);
      +					} else {
     -+						ctx->argv[0] = xstrdup(ctx->opt - 1);
     ++						ctx->argv[0] =
     ++							xstrdup(ctx->opt - 1);
      +					}
       					*(char *)ctx->argv[0] = '-';
       					goto unknown;
     @@ parse-options.h: 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), \
     -+}
     ++#define OPT_UNKNOWN(v, fn)          \
     ++	{                           \
     ++		.type = OPTION_END, \
     ++		.value = (v),       \
     ++		.strdup_fn = (fn),  \
     ++	}
      +
       /*
        * parse_options() will filter out the processed options and leave the
     @@ parse-options.h: 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;
       };
     @@ t/helper/test-free-unknown-options.c (new)
      +#include "parse-options.h"
      +#include "setup.h"
      +#include "strvec.h"
     ++#include "test-tool.h"
      +
      +static const char *const free_unknown_options_usage[] = {
     -+    "test-tool free-unknown-options",
     -+    NULL
     ++	"test-tool free-unknown-options", NULL
      +};
      +
     -+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();
     ++static char *strvec_push_wrapper(void *value, const char *str)
     ++{
     ++	struct strvec *sv = value;
     ++	return (char *)strvec_push(sv, str);
     ++}
      +
     -+    bool a, b;
     ++int cmd__free_unknown_options(int argc, const char **argv)
     ++{
     ++	struct strvec *unknown_opts = xmalloc(sizeof(struct strvec));
     ++	const char *prefix = setup_git_directory();
     ++	int a = 0, b = 0;
     ++	size_t i;
      +	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),
     ++		OPT_BOOL('b', "test-b", &b, N_("option b, only for test use")),
     ++		OPT_UNKNOWN(unknown_opts, strvec_push_wrapper),
      +	};
      +
     -+    parse_options(argc, argv, prefix, options,
     -+	free_unknown_options_usage, PARSE_OPT_KEEP_UNKNOWN_OPT);
     ++	strvec_init(unknown_opts);
     ++	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");
     ++	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);
     ++	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);
     ++
     ++	return 0;
      +}
     - \ No newline at end of file
      
       ## t/helper/test-tool.c ##
      @@ t/helper/test-tool.c: static struct test_cmd cmds[] = {
       	{ "parse-options-flags", cmd__parse_options_flags },
       	{ "parse-pathspec-file", cmd__parse_pathspec_file },
       	{ "parse-subcommand", cmd__parse_subcommand },
     -+	{ "free-unknown-options", cmd__free_unknown_options},
     ++	{ "free-unknown-options", cmd__free_unknown_options },
       	{ "partial-clone", cmd__partial_clone },
       	{ "path-utils", cmd__path_utils },
       	{ "path-walk", cmd__path_walk },
     @@ t/t0040-parse-options.sh: 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
     -+
      +test_expect_success 'free unknown options' '
      +	test-tool free-unknown-options -ac -bd \
      +	>output 2>output.err &&
     ++	cat >expect <<-\EOF &&
     ++	a = true
     ++	b = true
     ++	free unknown option: -c
     ++	free unknown option: -d
     ++	EOF
      +	test_cmp expect output &&
      +	test_must_be_empty output.err
      +'
 2:  7a5e2f29529 < -:  ----------- fix: replace bug where int was incorrectly used as bool
 3:  a9cbca6bed3 < -:  ----------- fix: use strvec_push_wrapper to prevent ubsan failure

 Makefile                             |  1 +
 parse-options.c                      | 23 ++++++++++++++-
 parse-options.h                      | 12 ++++++++
 t/helper/meson.build                 |  1 +
 t/helper/test-free-unknown-options.c | 42 ++++++++++++++++++++++++++++
 t/helper/test-tool.c                 |  1 +
 t/helper/test-tool.h                 |  1 +
 t/t0040-parse-options.sh             | 13 +++++++++
 8 files changed, 93 insertions(+), 1 deletion(-)
 create mode 100644 t/helper/test-free-unknown-options.c

index 8a7f1c76543..af8ea677b82 100644
@@ -822,6 +822,7 @@ TEST_BUILTINS_OBJS += test-online-cpus.o
 TEST_BUILTINS_OBJS += test-pack-mtimes.o
 TEST_BUILTINS_OBJS += test-parse-options.o
 TEST_BUILTINS_OBJS += test-parse-pathspec-file.o
+TEST_BUILTINS_OBJS += test-free-unknown-options.o
 TEST_BUILTINS_OBJS += test-partial-clone.o
 TEST_BUILTINS_OBJS += test-path-utils.o
 TEST_BUILTINS_OBJS += test-path-walk.o

index a9a39ecaef6..12721b83000 100644
@@ -638,6 +638,18 @@ 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)
+{
+	for (; options->type != OPTION_END; options++)
+		;
+	if (options->value && options->strdup_fn) {
+		ctx->unknown_opts = options->value;
+		ctx->strdup_fn = options->strdup_fn;
+		return;
+	}
+}
+
 static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
 				  int argc, const char **argv, const char *prefix,
 				  const struct option *options,

@@ -655,6 +667,7 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
 	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
 	ctx->flags = flags;
 	ctx->has_subcommands = has_subcommands(options);
+	set_strdup_fn(ctx, options);
 	if (!ctx->has_subcommands && (flags & PARSE_OPT_SUBCOMMAND_OPTIONAL))
 		BUG("Using PARSE_OPT_SUBCOMMAND_OPTIONAL without subcommands");
 	if (ctx->has_subcommands) {

@@ -981,7 +994,15 @@ 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:

index 91c3e3c29b3..57037bd381a 100644
@@ -77,6 +77,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
 typedef int parse_opt_subcommand_fn(int argc, const char **argv,
 				    const char *prefix, struct repository *repo);
 
+typedef char *parse_opt_strdup_fn(void *value, const char *s);
+
 /*
  * `type`::
  *   holds the type of the option, you must have an OPTION_END last in your

@@ -165,6 +167,7 @@ struct option {
 	parse_opt_ll_cb *ll_callback;
 	intptr_t extra;
 	parse_opt_subcommand_fn *subcommand_fn;
+	parse_opt_strdup_fn *strdup_fn;
 };
 
 #define OPT_BIT_F(s, l, v, h, b, f) { \

@@ -388,6 +391,13 @@ 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 +506,8 @@ 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,

index d2cabaa2bcf..476e3278176 100644
@@ -39,6 +39,7 @@ test_tool_sources = [
   'test-pack-mtimes.c',
   'test-parse-options.c',
   'test-parse-pathspec-file.c',
+  'test-free-unknown-options.c',
   'test-partial-clone.c',
   'test-path-utils.c',
   'test-path-walk.c',

index 50dc4dac4ed..5af5acae1cb 100644
@@ -51,6 +51,7 @@ static struct test_cmd cmds[] = {
 	{ "parse-options-flags", cmd__parse_options_flags },
 	{ "parse-pathspec-file", cmd__parse_pathspec_file },
 	{ "parse-subcommand", cmd__parse_subcommand },
+	{ "free-unknown-options", cmd__free_unknown_options },
 	{ "partial-clone", cmd__partial_clone },
 	{ "path-utils", cmd__path_utils },
 	{ "path-walk", cmd__path_walk },

index 6d62a5b53d9..33fa7828b9f 100644
@@ -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);

index ca55ea8228c..df221be5d18 100755
@@ -822,4 +822,17 @@ test_expect_success 'u16 limits range' '
 	test_grep "value 65536 for option .u16. not in range \[0,65535\]" err
 '
 
+test_expect_success 'free unknown options' '
+	test-tool free-unknown-options -ac -bd \
+	>output 2>output.err &&
+	cat >expect <<-\EOF &&
+	a = true
+	b = true
+	free unknown option: -c
+	free unknown option: -d
+	EOF
+	test_cmp expect output &&
+	test_must_be_empty output.err
+'
+
 test_done

base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75

gitgitgadget

Again, no need to quote the exact line number.

So does that mean that _every_ user of the "parse-options" interfaces now has to explicitly plug this memory leak when facing unknown options? That sounds rather undesirable, as there are so many users out there.

Hm. Is there any other usecase for the strdup_fn field that you can think about in the future? Otherwise it feels a bit overengineered from my perspective.

I unfortuntaely don't think the proposed solution works well. To really plug the memory leak everywhere, we would have to convert every single user of parse_options() to pass OPTION_UNKNOWN() instead of OPTION_END(), pass a strvec and clear that strvec at the end. It just doesn't quite feel like it's worth it to plug a single, bounded memory leak.

The most important problem that we're facing is that the allocated string has a lifetime longer than parse_options(), because it really only becomes relevant in the case where PARSE_OPT_KEEP_UNKNOWN_OPT is enabled. If so, the _caller_ of parse_options() will want to process the modified argv, and thus they have to be the ones responsible for freeing potentially-allocated memory once it's unused.

But that'd require us to keep some state. You solve this via the extra strvec, but that doesn't really look like a great solution to me. An alternative would be to expose the parse_options_ctx and let the caller eventually call parse_options_clear() or something like that. This would also require us to modify every caller, but the benefit would be that we now have a single place where we can always allocate memory in without having to worry about its lifetime.

All of that starts to become kind of involved though. So unless others disagree with my analysis I just don't think this edge case really is worth the complexity.

Thanks!

Patrick

Since the lifetime of argv last until the program terminates, a memory leak can only occur if parse_option is called multiple times and at least two of those calls use the PARSE_OPT_KEEP_UNKNOWN flag. In the other words, the memory leak only occurs when the statement ctx->argue[0] = xstrdup overwrites the result of a previous xstrdup call.

I think a simple approach is to add a marker to the string allocated by xstrdup , and before the next potential leaking ctx->argv[0] = xstrdup, check whether the string needs to be freed. Like we could allocate one more byte in the end of the string to store the marker.

I agree with this. We seem to be copying the string because argv is a const char** but would it really be so bad to modify the string that gets passed to us? We know that any bundled options that come before an unknown option cannot have a value or else what we see as the unknown option would be part of that value. So I think we could do

index 35fbb3b0d6..9e6e46da27 100644
@@ -924,12 +924,12 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
                                         if (internal_help && *ctx->opt == 'h')
                                                 goto show_usage;
 
-                                        /* fake a short option thing to hide the fact that we may have
+                                        /* move a short option thing to hide the fact that we may have
                                          * started to parse aggregated stuff
-                                         *
-                                         * This is leaky, too bad.
                                          */
-                                        ctx->argv[0] = xstrdup(ctx->opt - 1);
+                                        MOVE_ARRAY((char *)arg, ctx->opt - 1,
+                                                   strlen(ctx->opt) + 2);
+                                        ctx->argv[0] = arg;
                                         *(char *)ctx->argv[0] = '-';
                                         goto unknown;
                                 case PARSE_OPT_NON_OPTION:

Best Wishes

Phillip

On 09/05/2025 21:08，Phillip Wood <[email protected]> write：

From: Lidong Yan <[email protected]>

call parse_options twice and occasionally meet unknown option in the same place would cause memory leak, since the second xstrdup in parse_options_step would make the first xstrdup unreachable.

One solution is allocate one more magic byte for the unknown option to indicate that argv[?] stores a heap allocated arg. Assume for all arg, *((char *)arg - 1) is valid, we could put a magic number before the unknown option. Next time calling "xstrdup" will check the magic number first, and frees the previous heap allocated memory.

---
fix xstrdup leak in parse_short_opt

Pass a user defined strdup-like function in parse_opt_ctx to avoid memory leak.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1954%2Fbrandb97%2Ffix-parse-option-leak-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1954/brandb97/fix-parse-option-leak-v3
Pull-Request: https://github.com/git/git/pull/1954

Range-diff vs v2:

 1:  e7b4465b83e < -:  ----------- parse-options: fix xstrdup leak in parse_options_step parse-options:984
 -:  ----------- > 1:  475f7b5b1bd parse-options: fix memory leak when calling `parse_options`

 parse-options.c | 12 +++++++++++-
 parse-options.h |  2 ++
 2 files changed, 13 insertions(+), 1 deletion(-)

index a9a39ecaef6..84f6ae90c77 100644
@@ -886,6 +886,8 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 					 const char * const usagestr[])
 {
 	int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);
+	char *magic_ptr = NULL;
+	size_t opt_sz = 0;
 
 	/* we must reset ->opt, unknown short option leave it dangling */
 	ctx->opt = NULL;

@@ -981,7 +983,15 @@ 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);
+					magic_ptr = (char *)ctx->argv[0] - 1;
+					if (*magic_ptr == OPT_MAGIC)
+						free(magic_ptr);
+					opt_sz = strlen(ctx->opt - 1) + 1;
+					magic_ptr = xmalloc(opt_sz + 1);
+					*magic_ptr = OPT_MAGIC;
+					ctx->argv[0] = magic_ptr + 1;
+					memcpy((char *)ctx->argv[0],
+					       ctx->opt - 1, opt_sz);
 					*(char *)ctx->argv[0] = '-';
 					goto unknown;
 				case PARSE_OPT_NON_OPTION:

index 91c3e3c29b3..7fdd2e1097a 100644
@@ -477,6 +477,8 @@ static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,
 		BUG("option callback expects an argument"); \
 } while(0)
 
+#define OPT_MAGIC ((char)(0xee))
+
 /*----- incremental advanced APIs -----*/
 
 struct parse_opt_cmdmode_list;

base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75

gitgitgadget