{"thread":{"id":"63411","subject":"[PATCH 0/3] fix xstrdup leak in parse_short_opt","startedAt":"2025-05-07T02:33:26Z","lastAt":"2025-05-09T14:28:36Z","messageCount":14,"participants":["Lidong Yan via GitGitGadget","Patrick Steinhardt","lidongyan","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"517432","messageId":"pull.1954.git.git.1746585203.gitgitgadget@gmail.com","threadId":"63411","inReplyTo":null,"subject":"[PATCH 0/3] fix xstrdup leak in parse_short_opt","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-07T02:33:20Z","receivedAt":"2025-05-07T02:33:26Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Pass a usered defined strdup-like function a parse_opt_ctx to avoid memory\nleak.\n\nLidong Yan (3):\n  fix xstrdup leak in parse_short_opt\n  fix: replace bug where int was incorrectly used as bool\n  fix: use strvec_push_wrapper to prevent ubsan failure\n\n Makefile                             |  1 +\n parse-options.c                      | 23 ++++++++++++++-\n parse-options.h                      | 12 ++++++++\n t/helper/meson.build                 |  1 +\n t/helper/test-free-unknown-options.c | 42 ++++++++++++++++++++++++++++\n t/helper/test-tool.c                 |  1 +\n t/helper/test-tool.h                 |  1 +\n t/t0040-parse-options.sh             | 14 ++++++++++\n 8 files changed, 94 insertions(+), 1 deletion(-)\n create mode 100644 t/helper/test-free-unknown-options.c\n\n\nbase-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1954%2Fbrandb97%2Ffix-parse-option-leak-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1954/brandb97/fix-parse-option-leak-v1\nPull-Request: https://github.com/git/git/pull/1954\n-- \ngitgitgadget\n"},{"id":"517433","messageId":"b34445d166c9790e922c0fd6a7c4885031fb21bd.1746585203.git.gitgitgadget@gmail.com","threadId":"63411","inReplyTo":"pull.1954.git.git.1746585203.gitgitgadget@gmail.com","subject":"[PATCH 1/3] fix xstrdup leak in parse_short_opt","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-07T02:33:21Z","receivedAt":"2025-05-07T02:33:27Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n Makefile                             |  1 +\n parse-options.c                      | 17 +++++++++++++-\n parse-options.h                      | 12 ++++++++++\n t/helper/meson.build                 |  1 +\n t/helper/test-free-unknown-options.c | 35 ++++++++++++++++++++++++++++\n t/helper/test-tool.c                 |  1 +\n t/helper/test-tool.h                 |  1 +\n t/t0040-parse-options.sh             | 14 +++++++++++\n 8 files changed, 81 insertions(+), 1 deletion(-)\n create mode 100644 t/helper/test-free-unknown-options.c\n\ndiff --git a/Makefile b/Makefile\nindex 8a7f1c76543f..af8ea677b820 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -822,6 +822,7 @@ TEST_BUILTINS_OBJS += test-online-cpus.o\n TEST_BUILTINS_OBJS += test-pack-mtimes.o\n TEST_BUILTINS_OBJS += test-parse-options.o\n TEST_BUILTINS_OBJS += test-parse-pathspec-file.o\n+TEST_BUILTINS_OBJS += test-free-unknown-options.o\n TEST_BUILTINS_OBJS += test-partial-clone.o\n TEST_BUILTINS_OBJS += test-path-utils.o\n TEST_BUILTINS_OBJS += test-path-walk.o\ndiff --git a/parse-options.c b/parse-options.c\nindex a9a39ecaef6c..4279dfe4d313 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -638,6 +638,16 @@ static int has_subcommands(const struct option *options)\n \treturn 0;\n }\n \n+static void set_strdup_fn(struct parse_opt_ctx_t *ctx, const struct option *options) {\n+\tfor (; options->type != OPTION_END; options++)\n+\t\t;\n+\tif (options->value && options->strdup_fn) {\n+\t\tctx->unknown_opts = options->value;\n+\t\tctx->strdup_fn = options->strdup_fn;\n+\t\treturn;\n+\t}\n+}\n+\n static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n \t\t\t\t  int argc, const char **argv, const char *prefix,\n \t\t\t\t  const struct option *options,\n@@ -655,6 +665,7 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n \tctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n \tctx->flags = flags;\n \tctx->has_subcommands = has_subcommands(options);\n+\tset_strdup_fn(ctx, options);\n \tif (!ctx->has_subcommands && (flags & PARSE_OPT_SUBCOMMAND_OPTIONAL))\n \t\tBUG(\"Using PARSE_OPT_SUBCOMMAND_OPTIONAL without subcommands\");\n \tif (ctx->has_subcommands) {\n@@ -981,7 +992,11 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t *\n \t\t\t\t\t * This is leaky, too bad.\n \t\t\t\t\t */\n-\t\t\t\t\tctx->argv[0] = xstrdup(ctx->opt - 1);\n+\t\t\t\t\tif (ctx->unknown_opts && ctx->strdup_fn) {\n+\t\t\t\t\t\tctx->argv[0] = ctx->strdup_fn(ctx->unknown_opts, ctx->opt - 1);\n+\t\t\t\t\t} else {\n+\t\t\t\t\t\tctx->argv[0] = xstrdup(ctx->opt - 1);\n+\t\t\t\t\t}\n \t\t\t\t\t*(char *)ctx->argv[0] = '-';\n \t\t\t\t\tgoto unknown;\n \t\t\t\tcase PARSE_OPT_NON_OPTION:\ndiff --git a/parse-options.h b/parse-options.h\nindex 91c3e3c29b3d..af06a09abb8e 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -77,6 +77,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,\n typedef int parse_opt_subcommand_fn(int argc, const char **argv,\n \t\t\t\t    const char *prefix, struct repository *repo);\n \n+typedef char *parse_opt_strdup_fn(void *value, const char *s);\n+\n /*\n  * `type`::\n  *   holds the type of the option, you must have an OPTION_END last in your\n@@ -165,6 +167,7 @@ struct option {\n \tparse_opt_ll_cb *ll_callback;\n \tintptr_t extra;\n \tparse_opt_subcommand_fn *subcommand_fn;\n+\tparse_opt_strdup_fn *strdup_fn;\n };\n \n #define OPT_BIT_F(s, l, v, h, b, f) { \\\n@@ -388,6 +391,12 @@ static char *parse_options_noop_ignored_value MAYBE_UNUSED;\n }\n #define OPT_SUBCOMMAND(l, v, fn)    OPT_SUBCOMMAND_F((l), (v), (fn), 0)\n \n+#define OPT_UNKNOWN(v, fn) { \\\n+\t.type = OPTION_END, \\\n+\t.value = (v), \\\n+\t.strdup_fn = (fn), \\\n+}\n+\n /*\n  * parse_options() will filter out the processed options and leave the\n  * non-option arguments in argv[]. argv0 is assumed program name and\n@@ -496,6 +505,9 @@ struct parse_opt_ctx_t {\n \tconst char *prefix;\n \tconst char **alias_groups; /* must be in groups of 3 elements! */\n \tstruct parse_opt_cmdmode_list *cmdmode_list;\n+\n+\tvoid *unknown_opts;\n+\tparse_opt_strdup_fn *strdup_fn;\n };\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\ndiff --git a/t/helper/meson.build b/t/helper/meson.build\nindex d2cabaa2bcfc..476e32781761 100644\n--- a/t/helper/meson.build\n+++ b/t/helper/meson.build\n@@ -39,6 +39,7 @@ test_tool_sources = [\n   'test-pack-mtimes.c',\n   'test-parse-options.c',\n   'test-parse-pathspec-file.c',\n+  'test-free-unknown-options.c',\n   'test-partial-clone.c',\n   'test-path-utils.c',\n   'test-path-walk.c',\ndiff --git a/t/helper/test-free-unknown-options.c b/t/helper/test-free-unknown-options.c\nnew file mode 100644\nindex 000000000000..9d658115ba8f\n--- /dev/null\n+++ b/t/helper/test-free-unknown-options.c\n@@ -0,0 +1,35 @@\n+#include \"git-compat-util.h\"\n+#include \"parse-options.h\"\n+#include \"setup.h\"\n+#include \"strvec.h\"\n+\n+static const char *const free_unknown_options_usage[] = {\n+    \"test-tool free-unknown-options\",\n+    NULL\n+};\n+\n+int cmd__free_unknown_options(int argc, const char **argv) {\n+    struct strvec *unknown_opts = xmalloc(sizeof(struct strvec));\n+    strvec_init(unknown_opts);\n+    const char *prefix = setup_git_directory();\n+\n+    bool a, b;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL('a', \"test-a\", &a, N_(\"option a, only for test use\")),\n+\tOPT_BOOL('b', \"test-b\", &b, N_(\"option b, only for test use\")),\n+\tOPT_UNKNOWN(unknown_opts, (parse_opt_strdup_fn *)&strvec_push),\n+\t};\n+\n+    parse_options(argc, argv, prefix, options,\n+\tfree_unknown_options_usage, PARSE_OPT_KEEP_UNKNOWN_OPT);\n+\n+    printf(\"a = %s\\n\", a? \"true\": \"false\");\n+    printf(\"b = %s\\n\", b? \"true\": \"false\");\n+\n+    int i;\n+    for (i = 0; i < unknown_opts->nr; i++) {\n+\tprintf(\"free unknown option: %s\\n\", unknown_opts->v[i]);\n+    }\n+    strvec_clear(unknown_opts);\n+    free(unknown_opts);\n+}\n\\ No newline at end of file\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 50dc4dac4ed6..79ec4f9cda07 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -51,6 +51,7 @@ static struct test_cmd cmds[] = {\n \t{ \"parse-options-flags\", cmd__parse_options_flags },\n \t{ \"parse-pathspec-file\", cmd__parse_pathspec_file },\n \t{ \"parse-subcommand\", cmd__parse_subcommand },\n+\t{ \"free-unknown-options\", cmd__free_unknown_options},\n \t{ \"partial-clone\", cmd__partial_clone },\n \t{ \"path-utils\", cmd__path_utils },\n \t{ \"path-walk\", cmd__path_walk },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 6d62a5b53d95..33fa7828b9f6 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -44,6 +44,7 @@ int cmd__parse_options(int argc, const char **argv);\n int cmd__parse_options_flags(int argc, const char **argv);\n int cmd__parse_pathspec_file(int argc, const char** argv);\n int cmd__parse_subcommand(int argc, const char **argv);\n+int cmd__free_unknown_options(int argc, const char **argv);\n int cmd__partial_clone(int argc, const char **argv);\n int cmd__path_utils(int argc, const char **argv);\n int cmd__path_walk(int argc, const char **argv);\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex ca55ea8228c3..773f54103fd3 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -822,4 +822,18 @@ test_expect_success 'u16 limits range' '\n \ttest_grep \"value 65536 for option .u16. not in range \\[0,65535\\]\" err\n '\n \n+cat >expect <<\\EOF\n+a = true\n+b = true\n+free unknown option: -c\n+free unknown option: -d\n+EOF\n+\n+test_expect_success 'free unknown options' '\n+\ttest-tool free-unknown-options -ac -bd \\\n+\t>output 2>output.err &&\n+\ttest_cmp expect output &&\n+\ttest_must_be_empty output.err\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"517434","messageId":"7a5e2f295296265379551584a271a409a0ce3153.1746585203.git.gitgitgadget@gmail.com","threadId":"63411","inReplyTo":"pull.1954.git.git.1746585203.gitgitgadget@gmail.com","subject":"[PATCH 2/3] fix: replace bug where int was incorrectly used as bool","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-07T02:33:22Z","receivedAt":"2025-05-07T02:33:29Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n parse-options.c                      | 12 ++++++--\n parse-options.h                      | 12 ++++----\n t/helper/test-free-unknown-options.c | 43 ++++++++++++++--------------\n t/helper/test-tool.c                 |  2 +-\n 4 files changed, 38 insertions(+), 31 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 4279dfe4d313..12721b83000a 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -638,7 +638,9 @@ static int has_subcommands(const struct option *options)\n \treturn 0;\n }\n \n-static void set_strdup_fn(struct parse_opt_ctx_t *ctx, const struct option *options) {\n+static void set_strdup_fn(struct parse_opt_ctx_t *ctx,\n+\t\t\t  const struct option *options)\n+{\n \tfor (; options->type != OPTION_END; options++)\n \t\t;\n \tif (options->value && options->strdup_fn) {\n@@ -993,9 +995,13 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t * This is leaky, too bad.\n \t\t\t\t\t */\n \t\t\t\t\tif (ctx->unknown_opts && ctx->strdup_fn) {\n-\t\t\t\t\t\tctx->argv[0] = ctx->strdup_fn(ctx->unknown_opts, ctx->opt - 1);\n+\t\t\t\t\t\tctx->argv[0] =\n+\t\t\t\t\t\t\tctx->strdup_fn(ctx->unknown_opts,\n+\t\t\t\t\t\t\t\t       ctx->opt -\n+\t\t\t\t\t\t\t\t\t       1);\n \t\t\t\t\t} else {\n-\t\t\t\t\t\tctx->argv[0] = xstrdup(ctx->opt - 1);\n+\t\t\t\t\t\tctx->argv[0] =\n+\t\t\t\t\t\t\txstrdup(ctx->opt - 1);\n \t\t\t\t\t}\n \t\t\t\t\t*(char *)ctx->argv[0] = '-';\n \t\t\t\t\tgoto unknown;\ndiff --git a/parse-options.h b/parse-options.h\nindex af06a09abb8e..57037bd381a3 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -391,11 +391,12 @@ static char *parse_options_noop_ignored_value MAYBE_UNUSED;\n }\n #define OPT_SUBCOMMAND(l, v, fn)    OPT_SUBCOMMAND_F((l), (v), (fn), 0)\n \n-#define OPT_UNKNOWN(v, fn) { \\\n-\t.type = OPTION_END, \\\n-\t.value = (v), \\\n-\t.strdup_fn = (fn), \\\n-}\n+#define OPT_UNKNOWN(v, fn)          \\\n+\t{                           \\\n+\t\t.type = OPTION_END, \\\n+\t\t.value = (v),       \\\n+\t\t.strdup_fn = (fn),  \\\n+\t}\n \n /*\n  * parse_options() will filter out the processed options and leave the\n@@ -505,7 +506,6 @@ struct parse_opt_ctx_t {\n \tconst char *prefix;\n \tconst char **alias_groups; /* must be in groups of 3 elements! */\n \tstruct parse_opt_cmdmode_list *cmdmode_list;\n-\n \tvoid *unknown_opts;\n \tparse_opt_strdup_fn *strdup_fn;\n };\ndiff --git a/t/helper/test-free-unknown-options.c b/t/helper/test-free-unknown-options.c\nindex 9d658115ba8f..59d732da23ca 100644\n--- a/t/helper/test-free-unknown-options.c\n+++ b/t/helper/test-free-unknown-options.c\n@@ -2,34 +2,35 @@\n #include \"parse-options.h\"\n #include \"setup.h\"\n #include \"strvec.h\"\n+#include \"test-tool.h\"\n \n static const char *const free_unknown_options_usage[] = {\n-    \"test-tool free-unknown-options\",\n-    NULL\n+\t\"test-tool free-unknown-options\", NULL\n };\n \n-int cmd__free_unknown_options(int argc, const char **argv) {\n-    struct strvec *unknown_opts = xmalloc(sizeof(struct strvec));\n-    strvec_init(unknown_opts);\n-    const char *prefix = setup_git_directory();\n-\n-    bool a, b;\n+int cmd__free_unknown_options(int argc, const char **argv)\n+{\n+\tstruct strvec *unknown_opts = xmalloc(sizeof(struct strvec));\n+\tconst char *prefix = setup_git_directory();\n+\tint a = 0, b = 0;\n+\tsize_t i;\n \tstruct option options[] = {\n \t\tOPT_BOOL('a', \"test-a\", &a, N_(\"option a, only for test use\")),\n-\tOPT_BOOL('b', \"test-b\", &b, N_(\"option b, only for test use\")),\n-\tOPT_UNKNOWN(unknown_opts, (parse_opt_strdup_fn *)&strvec_push),\n+\t\tOPT_BOOL('b', \"test-b\", &b, N_(\"option b, only for test use\")),\n+\t\tOPT_UNKNOWN(unknown_opts, (parse_opt_strdup_fn *)&strvec_push),\n \t};\n \n-    parse_options(argc, argv, prefix, options,\n-\tfree_unknown_options_usage, PARSE_OPT_KEEP_UNKNOWN_OPT);\n+\tstrvec_init(unknown_opts);\n+\tparse_options(argc, argv, prefix, options, free_unknown_options_usage,\n+\t\t      PARSE_OPT_KEEP_UNKNOWN_OPT);\n+\n+\tprintf(\"a = %s\\n\", a ? \"true\" : \"false\");\n+\tprintf(\"b = %s\\n\", b ? \"true\" : \"false\");\n \n-    printf(\"a = %s\\n\", a? \"true\": \"false\");\n-    printf(\"b = %s\\n\", b? \"true\": \"false\");\n+\tfor (i = 0; i < unknown_opts->nr; i++)\n+\t\tprintf(\"free unknown option: %s\\n\", unknown_opts->v[i]);\n+\tstrvec_clear(unknown_opts);\n+\tfree(unknown_opts);\n \n-    int i;\n-    for (i = 0; i < unknown_opts->nr; i++) {\n-\tprintf(\"free unknown option: %s\\n\", unknown_opts->v[i]);\n-    }\n-    strvec_clear(unknown_opts);\n-    free(unknown_opts);\n-}\n\\ No newline at end of file\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 79ec4f9cda07..5af5acae1cb3 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -51,7 +51,7 @@ static struct test_cmd cmds[] = {\n \t{ \"parse-options-flags\", cmd__parse_options_flags },\n \t{ \"parse-pathspec-file\", cmd__parse_pathspec_file },\n \t{ \"parse-subcommand\", cmd__parse_subcommand },\n-\t{ \"free-unknown-options\", cmd__free_unknown_options},\n+\t{ \"free-unknown-options\", cmd__free_unknown_options },\n \t{ \"partial-clone\", cmd__partial_clone },\n \t{ \"path-utils\", cmd__path_utils },\n \t{ \"path-walk\", cmd__path_walk },\n-- \ngitgitgadget\n\n"},{"id":"517435","messageId":"a9cbca6bed308f2b563eca5d7707c6bc30aa4603.1746585203.git.gitgitgadget@gmail.com","threadId":"63411","inReplyTo":"pull.1954.git.git.1746585203.gitgitgadget@gmail.com","subject":"[PATCH 3/3] fix: use strvec_push_wrapper to prevent ubsan failure","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-07T02:33:23Z","receivedAt":"2025-05-07T02:33:30Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n t/helper/test-free-unknown-options.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-free-unknown-options.c b/t/helper/test-free-unknown-options.c\nindex 59d732da23ca..7369dfe379d6 100644\n--- a/t/helper/test-free-unknown-options.c\n+++ b/t/helper/test-free-unknown-options.c\n@@ -8,6 +8,12 @@ static const char *const free_unknown_options_usage[] = {\n \t\"test-tool free-unknown-options\", NULL\n };\n \n+static char *strvec_push_wrapper(void *value, const char *str)\n+{\n+\tstruct strvec *sv = value;\n+\treturn (char *)strvec_push(sv, str);\n+}\n+\n int cmd__free_unknown_options(int argc, const char **argv)\n {\n \tstruct strvec *unknown_opts = xmalloc(sizeof(struct strvec));\n@@ -17,7 +23,7 @@ int cmd__free_unknown_options(int argc, const char **argv)\n \tstruct option options[] = {\n \t\tOPT_BOOL('a', \"test-a\", &a, N_(\"option a, only for test use\")),\n \t\tOPT_BOOL('b', \"test-b\", &b, N_(\"option b, only for test use\")),\n-\t\tOPT_UNKNOWN(unknown_opts, (parse_opt_strdup_fn *)&strvec_push),\n+\t\tOPT_UNKNOWN(unknown_opts, strvec_push_wrapper),\n \t};\n \n \tstrvec_init(unknown_opts);\n-- \ngitgitgadget\n"},{"id":"517460","messageId":"aBsRxv9QkFTHeG3V@pks.im","threadId":"63411","inReplyTo":"b34445d166c9790e922c0fd6a7c4885031fb21bd.1746585203.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] fix xstrdup leak in parse_short_opt","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-07T07:54:46Z","receivedAt":"2025-05-07T07:54:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 07, 2025 at 02:33:21AM +0000, Lidong Yan via GitGitGadget wrote:\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nThis commit is missing an explanation of the problem. When does it\ntrigger? Which subsystem triggers it? Why didn't we observe the issue\nbeforehand? And given that the solution seems to be a bit more complex\nit should also explain how you're fixing the issue.\n\nFurthermore, the subject of such a commit should be prefixed with the\nsubsystem in which you're fixing the issue. So in your case,\n\"parse-options:\".\n\nI'd recommend reading through \"Documentation/MyFirstContribution.adoc\",\nwhich explains all of this.\n\n> diff --git a/parse-options.c b/parse-options.c\n> index a9a39ecaef6c..4279dfe4d313 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -638,6 +638,16 @@ static int has_subcommands(const struct option *options)\n>  \treturn 0;\n>  }\n>  \n> +static void set_strdup_fn(struct parse_opt_ctx_t *ctx, const struct option *options) {\n\nStyle: the opening brace should be on its own line.\n\n> +\tfor (; options->type != OPTION_END; options++)\n> +\t\t;\n> +\tif (options->value && options->strdup_fn) {\n> +\t\tctx->unknown_opts = options->value;\n> +\t\tctx->strdup_fn = options->strdup_fn;\n> +\t\treturn;\n> +\t}\n\nIt's not really clear what this is doing. Is the intent to only change\nthis this value for the last option? If so, why?\n\n> @@ -981,7 +992,11 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n>  \t\t\t\t\t *\n>  \t\t\t\t\t * This is leaky, too bad.\n>  \t\t\t\t\t */\n> -\t\t\t\t\tctx->argv[0] = xstrdup(ctx->opt - 1);\n> +\t\t\t\t\tif (ctx->unknown_opts && ctx->strdup_fn) {\n> +\t\t\t\t\t\tctx->argv[0] = ctx->strdup_fn(ctx->unknown_opts, ctx->opt - 1);\n> +\t\t\t\t\t} else {\n> +\t\t\t\t\t\tctx->argv[0] = xstrdup(ctx->opt - 1);\n> +\t\t\t\t\t}\n>  \t\t\t\t\t*(char *)ctx->argv[0] = '-';\n>  \t\t\t\t\tgoto unknown;\n>  \t\t\t\tcase PARSE_OPT_NON_OPTION:\n\nHm. I'm at a loss what we're solving here. All of this really should be\nexplained in the commit message to give context to the reviewer.\n\n> diff --git a/parse-options.h b/parse-options.h\n> index 91c3e3c29b3d..af06a09abb8e 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -388,6 +391,12 @@ static char *parse_options_noop_ignored_value MAYBE_UNUSED;\n>  }\n>  #define OPT_SUBCOMMAND(l, v, fn)    OPT_SUBCOMMAND_F((l), (v), (fn), 0)\n>  \n> +#define OPT_UNKNOWN(v, fn) { \\\n> +\t.type = OPTION_END, \\\n> +\t.value = (v), \\\n> +\t.strdup_fn = (fn), \\\n> +}\n> +\n>  /*\n>   * parse_options() will filter out the processed options and leave the\n>   * non-option arguments in argv[]. argv0 is assumed program name and\n> @@ -496,6 +505,9 @@ struct parse_opt_ctx_t {\n>  \tconst char *prefix;\n>  \tconst char **alias_groups; /* must be in groups of 3 elements! */\n>  \tstruct parse_opt_cmdmode_list *cmdmode_list;\n> +\n> +\tvoid *unknown_opts;\n> +\tparse_opt_strdup_fn *strdup_fn;\n>  };\n>  \n>  void parse_options_start(struct parse_opt_ctx_t *ctx,\n\nOkay. So the intent seems to be that we start storing any options that\nwe don't understand?\n\n> diff --git a/t/helper/test-free-unknown-options.c b/t/helper/test-free-unknown-options.c\n> new file mode 100644\n> index 000000000000..9d658115ba8f\n> --- /dev/null\n> +++ b/t/helper/test-free-unknown-options.c\n> @@ -0,0 +1,35 @@\n> +#include \"git-compat-util.h\"\n> +#include \"parse-options.h\"\n> +#include \"setup.h\"\n> +#include \"strvec.h\"\n> +\n> +static const char *const free_unknown_options_usage[] = {\n> +    \"test-tool free-unknown-options\",\n> +    NULL\n> +};\n\nCan't we expand `test-tool parse-options` instead of introducing a new\nhelper?\n\n> +int cmd__free_unknown_options(int argc, const char **argv) {\n> +    struct strvec *unknown_opts = xmalloc(sizeof(struct strvec));\n> +    strvec_init(unknown_opts);\n> +    const char *prefix = setup_git_directory();\n> +\n> +    bool a, b;\n> +\tstruct option options[] = {\n> +\t\tOPT_BOOL('a', \"test-a\", &a, N_(\"option a, only for test use\")),\n> +\tOPT_BOOL('b', \"test-b\", &b, N_(\"option b, only for test use\")),\n> +\tOPT_UNKNOWN(unknown_opts, (parse_opt_strdup_fn *)&strvec_push),\n> +\t};\n> +\n> +    parse_options(argc, argv, prefix, options,\n> +\tfree_unknown_options_usage, PARSE_OPT_KEEP_UNKNOWN_OPT);\n> +\n> +    printf(\"a = %s\\n\", a? \"true\": \"false\");\n> +    printf(\"b = %s\\n\", b? \"true\": \"false\");\n> +\n> +    int i;\n> +    for (i = 0; i < unknown_opts->nr; i++) {\n> +\tprintf(\"free unknown option: %s\\n\", unknown_opts->v[i]);\n> +    }\n> +    strvec_clear(unknown_opts);\n> +    free(unknown_opts);\n> +}\n> \\ No newline at end of file\n\nMissing newline.\n\n> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\n> index 6d62a5b53d95..33fa7828b9f6 100644\n> --- a/t/helper/test-tool.h\n> +++ b/t/helper/test-tool.h\n> @@ -44,6 +44,7 @@ int cmd__parse_options(int argc, const char **argv);\n>  int cmd__parse_options_flags(int argc, const char **argv);\n>  int cmd__parse_pathspec_file(int argc, const char** argv);\n>  int cmd__parse_subcommand(int argc, const char **argv);\n> +int cmd__free_unknown_options(int argc, const char **argv);\n>  int cmd__partial_clone(int argc, const char **argv);\n>  int cmd__path_utils(int argc, const char **argv);\n>  int cmd__path_walk(int argc, const char **argv);\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index ca55ea8228c3..773f54103fd3 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -822,4 +822,18 @@ test_expect_success 'u16 limits range' '\n>  \ttest_grep \"value 65536 for option .u16. not in range \\[0,65535\\]\" err\n>  '\n>  \n> +cat >expect <<\\EOF\n> +a = true\n> +b = true\n> +free unknown option: -c\n> +free unknown option: -d\n> +EOF\n\nThis should be done inside `test_expect_success` itself.\n\n> +test_expect_success 'free unknown options' '\n> +\ttest-tool free-unknown-options -ac -bd \\\n> +\t>output 2>output.err &&\n> +\ttest_cmp expect output &&\n> +\ttest_must_be_empty output.err\n> +'\n> +\n>  test_done\n\nOkay, so the memory leak seems to happen when handling unknown options\nindeed, as I started to expect after the middle of this patch. In any\ncase, this commit really needs to be polished to have a proper commit\nmessage that sets the stage for the reviewer.\n\nPatrick\n"},{"id":"517461","messageId":"aBsRzPvrMrDtEVli@pks.im","threadId":"63411","inReplyTo":"7a5e2f295296265379551584a271a409a0ce3153.1746585203.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] fix: replace bug where int was incorrectly used as bool","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-07T07:54:52Z","receivedAt":"2025-05-07T07:54:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 07, 2025 at 02:33:22AM +0000, Lidong Yan via GitGitGadget wrote:\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n> \n> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nThe same is true here -- this needs to have a proper commit subject and\nmessage.\n\n> diff --git a/parse-options.c b/parse-options.c\n> index 4279dfe4d313..12721b83000a 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -638,7 +638,9 @@ static int has_subcommands(const struct option *options)\n>  \treturn 0;\n>  }\n>  \n> -static void set_strdup_fn(struct parse_opt_ctx_t *ctx, const struct option *options) {\n> +static void set_strdup_fn(struct parse_opt_ctx_t *ctx,\n> +\t\t\t  const struct option *options)\n> +{\n>  \tfor (; options->type != OPTION_END; options++)\n>  \t\t;\n>  \tif (options->value && options->strdup_fn) {\n\nPlease refrain from doing code style cleanups of code you have just\nintroduced in the preceding commit.\n\nPatrick\n"},{"id":"517462","messageId":"aBsR0CUusW0Fo4Ef@pks.im","threadId":"63411","inReplyTo":"a9cbca6bed308f2b563eca5d7707c6bc30aa4603.1746585203.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] fix: use strvec_push_wrapper to prevent ubsan failure","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-07T07:54:56Z","receivedAt":"2025-05-07T07:54:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 07, 2025 at 02:33:23AM +0000, Lidong Yan via GitGitGadget wrote:\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n> \n> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nThis looks like another fixup-style commit. I assume that all of this\nreally should be a single commit, as the latter two commits don't seem\nto do anything new compared to the first commit.\n\nPatrick\n"},{"id":"517476","messageId":"1CEABFA7-474C-4253-BB92-E3F153213F5C@smail.nju.edu.cn","threadId":"63411","inReplyTo":"aBsR0CUusW0Fo4Ef@pks.im","subject":"Re: [PATCH 3/3] fix: use strvec_push_wrapper to prevent ubsan failure","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-07T12:19:29Z","receivedAt":"2025-05-07T12:19:58Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Thank you for your suggestion. I will squash all the commits and include a detailed description in the next submission.\n\nIn 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.\n\nOn 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.\n\n> 2025年5月7日 15:54，Patrick Steinhardt <ps@pks.im> 写道：\n> \n> On Wed, May 07, 2025 at 02:33:23AM +0000, Lidong Yan via GitGitGadget wrote:\n>> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>> \n>> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n> \n> This looks like another fixup-style commit. I assume that all of this\n> really should be a single commit, as the latter two commits don't seem\n> to do anything new compared to the first commit.\n> \n> Patrick\n> \n\n"},{"id":"517487","messageId":"pull.1954.v2.git.git.1746624294017.gitgitgadget@gmail.com","threadId":"63411","inReplyTo":"pull.1954.git.git.1746585203.gitgitgadget@gmail.com","subject":"[PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-07T13:24:53Z","receivedAt":"2025-05-07T13:24:57Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nMost Git commands use parse_options to parse options, but parse_options\ncauses a memory leak when it encounters unknown short options. The leak\noccurs at line 984 in parse-options.c, where git allocates some spaces\nto store the unknown short options and will never release those spaces.\n\nTo address this issue, users can be allowed to provide a custom function\nto allocate memory for unknown options. For example, users can use\nstrvec_push to allocate memory for unknown options and later call\nstrvec_clear at the end of the Git command to release the memory, thereby\navoiding the memory leak.\n\nThis commit allows users to provide a custom allocation function for\nunknown options via the strdup_fn field in the last struct option. For\nconvenience, this commit also introduces a new macro, OPT_UNKNOWN, which\nbehaves like OPT_END but takes an additional argument for the allocation\nfunction. parse_options could get the custom allocation function in struct\nparse_opt_ctx_t by using set_strdup_fn. A simple example to use\nOPT_UNKNOWN is put into t/helper/test-free-unknown-options.c\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    fix xstrdup leak in parse_short_opt\n    \n    Pass a user defined strdup-like function in parse_opt_ctx to avoid\n    memory leak.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1954%2Fbrandb97%2Ffix-parse-option-leak-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1954/brandb97/fix-parse-option-leak-v2\nPull-Request: https://github.com/git/git/pull/1954\n\nRange-diff vs v1:\n\n 1:  b34445d166c ! 1:  e7b4465b83e fix xstrdup leak in parse_short_opt\n     @@ Metadata\n      Author: Lidong Yan <502024330056@smail.nju.edu.cn>\n      \n       ## Commit message ##\n     -    fix xstrdup leak in parse_short_opt\n     +    parse-options: fix xstrdup leak in parse_options_step parse-options:984\n     +\n     +    Most Git commands use parse_options to parse options, but parse_options\n     +    causes a memory leak when it encounters unknown short options. The leak\n     +    occurs at line 984 in parse-options.c, where git allocates some spaces\n     +    to store the unknown short options and will never release those spaces.\n     +\n     +    To address this issue, users can be allowed to provide a custom function\n     +    to allocate memory for unknown options. For example, users can use\n     +    strvec_push to allocate memory for unknown options and later call\n     +    strvec_clear at the end of the Git command to release the memory, thereby\n     +    avoiding the memory leak.\n     +\n     +    This commit allows users to provide a custom allocation function for\n     +    unknown options via the strdup_fn field in the last struct option. For\n     +    convenience, this commit also introduces a new macro, OPT_UNKNOWN, which\n     +    behaves like OPT_END but takes an additional argument for the allocation\n     +    function. parse_options could get the custom allocation function in struct\n     +    parse_opt_ctx_t by using set_strdup_fn. A simple example to use\n     +    OPT_UNKNOWN is put into t/helper/test-free-unknown-options.c\n      \n          Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n      \n     @@ parse-options.c: static int has_subcommands(const struct option *options)\n       \treturn 0;\n       }\n       \n     -+static void set_strdup_fn(struct parse_opt_ctx_t *ctx, const struct option *options) {\n     ++static void set_strdup_fn(struct parse_opt_ctx_t *ctx,\n     ++\t\t\t  const struct option *options)\n     ++{\n      +\tfor (; options->type != OPTION_END; options++)\n      +\t\t;\n      +\tif (options->value && options->strdup_fn) {\n     @@ parse-options.c: enum parse_opt_result parse_options_step(struct parse_opt_ctx_t\n       \t\t\t\t\t */\n      -\t\t\t\t\tctx->argv[0] = xstrdup(ctx->opt - 1);\n      +\t\t\t\t\tif (ctx->unknown_opts && ctx->strdup_fn) {\n     -+\t\t\t\t\t\tctx->argv[0] = ctx->strdup_fn(ctx->unknown_opts, ctx->opt - 1);\n     ++\t\t\t\t\t\tctx->argv[0] =\n     ++\t\t\t\t\t\t\tctx->strdup_fn(ctx->unknown_opts,\n     ++\t\t\t\t\t\t\t\t       ctx->opt -\n     ++\t\t\t\t\t\t\t\t\t       1);\n      +\t\t\t\t\t} else {\n     -+\t\t\t\t\t\tctx->argv[0] = xstrdup(ctx->opt - 1);\n     ++\t\t\t\t\t\tctx->argv[0] =\n     ++\t\t\t\t\t\t\txstrdup(ctx->opt - 1);\n      +\t\t\t\t\t}\n       \t\t\t\t\t*(char *)ctx->argv[0] = '-';\n       \t\t\t\t\tgoto unknown;\n     @@ parse-options.h: static char *parse_options_noop_ignored_value MAYBE_UNUSED;\n       }\n       #define OPT_SUBCOMMAND(l, v, fn)    OPT_SUBCOMMAND_F((l), (v), (fn), 0)\n       \n     -+#define OPT_UNKNOWN(v, fn) { \\\n     -+\t.type = OPTION_END, \\\n     -+\t.value = (v), \\\n     -+\t.strdup_fn = (fn), \\\n     -+}\n     ++#define OPT_UNKNOWN(v, fn)          \\\n     ++\t{                           \\\n     ++\t\t.type = OPTION_END, \\\n     ++\t\t.value = (v),       \\\n     ++\t\t.strdup_fn = (fn),  \\\n     ++\t}\n      +\n       /*\n        * parse_options() will filter out the processed options and leave the\n     @@ parse-options.h: struct parse_opt_ctx_t {\n       \tconst char *prefix;\n       \tconst char **alias_groups; /* must be in groups of 3 elements! */\n       \tstruct parse_opt_cmdmode_list *cmdmode_list;\n     -+\n      +\tvoid *unknown_opts;\n      +\tparse_opt_strdup_fn *strdup_fn;\n       };\n     @@ t/helper/test-free-unknown-options.c (new)\n      +#include \"parse-options.h\"\n      +#include \"setup.h\"\n      +#include \"strvec.h\"\n     ++#include \"test-tool.h\"\n      +\n      +static const char *const free_unknown_options_usage[] = {\n     -+    \"test-tool free-unknown-options\",\n     -+    NULL\n     ++\t\"test-tool free-unknown-options\", NULL\n      +};\n      +\n     -+int cmd__free_unknown_options(int argc, const char **argv) {\n     -+    struct strvec *unknown_opts = xmalloc(sizeof(struct strvec));\n     -+    strvec_init(unknown_opts);\n     -+    const char *prefix = setup_git_directory();\n     ++static char *strvec_push_wrapper(void *value, const char *str)\n     ++{\n     ++\tstruct strvec *sv = value;\n     ++\treturn (char *)strvec_push(sv, str);\n     ++}\n      +\n     -+    bool a, b;\n     ++int cmd__free_unknown_options(int argc, const char **argv)\n     ++{\n     ++\tstruct strvec *unknown_opts = xmalloc(sizeof(struct strvec));\n     ++\tconst char *prefix = setup_git_directory();\n     ++\tint a = 0, b = 0;\n     ++\tsize_t i;\n      +\tstruct option options[] = {\n      +\t\tOPT_BOOL('a', \"test-a\", &a, N_(\"option a, only for test use\")),\n     -+\tOPT_BOOL('b', \"test-b\", &b, N_(\"option b, only for test use\")),\n     -+\tOPT_UNKNOWN(unknown_opts, (parse_opt_strdup_fn *)&strvec_push),\n     ++\t\tOPT_BOOL('b', \"test-b\", &b, N_(\"option b, only for test use\")),\n     ++\t\tOPT_UNKNOWN(unknown_opts, strvec_push_wrapper),\n      +\t};\n      +\n     -+    parse_options(argc, argv, prefix, options,\n     -+\tfree_unknown_options_usage, PARSE_OPT_KEEP_UNKNOWN_OPT);\n     ++\tstrvec_init(unknown_opts);\n     ++\tparse_options(argc, argv, prefix, options, free_unknown_options_usage,\n     ++\t\t      PARSE_OPT_KEEP_UNKNOWN_OPT);\n      +\n     -+    printf(\"a = %s\\n\", a? \"true\": \"false\");\n     -+    printf(\"b = %s\\n\", b? \"true\": \"false\");\n     ++\tprintf(\"a = %s\\n\", a ? \"true\" : \"false\");\n     ++\tprintf(\"b = %s\\n\", b ? \"true\" : \"false\");\n      +\n     -+    int i;\n     -+    for (i = 0; i < unknown_opts->nr; i++) {\n     -+\tprintf(\"free unknown option: %s\\n\", unknown_opts->v[i]);\n     -+    }\n     -+    strvec_clear(unknown_opts);\n     -+    free(unknown_opts);\n     ++\tfor (i = 0; i < unknown_opts->nr; i++)\n     ++\t\tprintf(\"free unknown option: %s\\n\", unknown_opts->v[i]);\n     ++\tstrvec_clear(unknown_opts);\n     ++\tfree(unknown_opts);\n     ++\n     ++\treturn 0;\n      +}\n     - \\ No newline at end of file\n      \n       ## t/helper/test-tool.c ##\n      @@ t/helper/test-tool.c: static struct test_cmd cmds[] = {\n       \t{ \"parse-options-flags\", cmd__parse_options_flags },\n       \t{ \"parse-pathspec-file\", cmd__parse_pathspec_file },\n       \t{ \"parse-subcommand\", cmd__parse_subcommand },\n     -+\t{ \"free-unknown-options\", cmd__free_unknown_options},\n     ++\t{ \"free-unknown-options\", cmd__free_unknown_options },\n       \t{ \"partial-clone\", cmd__partial_clone },\n       \t{ \"path-utils\", cmd__path_utils },\n       \t{ \"path-walk\", cmd__path_walk },\n     @@ t/t0040-parse-options.sh: test_expect_success 'u16 limits range' '\n       \ttest_grep \"value 65536 for option .u16. not in range \\[0,65535\\]\" err\n       '\n       \n     -+cat >expect <<\\EOF\n     -+a = true\n     -+b = true\n     -+free unknown option: -c\n     -+free unknown option: -d\n     -+EOF\n     -+\n      +test_expect_success 'free unknown options' '\n      +\ttest-tool free-unknown-options -ac -bd \\\n      +\t>output 2>output.err &&\n     ++\tcat >expect <<-\\EOF &&\n     ++\ta = true\n     ++\tb = true\n     ++\tfree unknown option: -c\n     ++\tfree unknown option: -d\n     ++\tEOF\n      +\ttest_cmp expect output &&\n      +\ttest_must_be_empty output.err\n      +'\n 2:  7a5e2f29529 < -:  ----------- fix: replace bug where int was incorrectly used as bool\n 3:  a9cbca6bed3 < -:  ----------- fix: use strvec_push_wrapper to prevent ubsan failure\n\n\n Makefile                             |  1 +\n parse-options.c                      | 23 ++++++++++++++-\n parse-options.h                      | 12 ++++++++\n t/helper/meson.build                 |  1 +\n t/helper/test-free-unknown-options.c | 42 ++++++++++++++++++++++++++++\n t/helper/test-tool.c                 |  1 +\n t/helper/test-tool.h                 |  1 +\n t/t0040-parse-options.sh             | 13 +++++++++\n 8 files changed, 93 insertions(+), 1 deletion(-)\n create mode 100644 t/helper/test-free-unknown-options.c\n\ndiff --git a/Makefile b/Makefile\nindex 8a7f1c76543..af8ea677b82 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -822,6 +822,7 @@ TEST_BUILTINS_OBJS += test-online-cpus.o\n TEST_BUILTINS_OBJS += test-pack-mtimes.o\n TEST_BUILTINS_OBJS += test-parse-options.o\n TEST_BUILTINS_OBJS += test-parse-pathspec-file.o\n+TEST_BUILTINS_OBJS += test-free-unknown-options.o\n TEST_BUILTINS_OBJS += test-partial-clone.o\n TEST_BUILTINS_OBJS += test-path-utils.o\n TEST_BUILTINS_OBJS += test-path-walk.o\ndiff --git a/parse-options.c b/parse-options.c\nindex a9a39ecaef6..12721b83000 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -638,6 +638,18 @@ static int has_subcommands(const struct option *options)\n \treturn 0;\n }\n \n+static void set_strdup_fn(struct parse_opt_ctx_t *ctx,\n+\t\t\t  const struct option *options)\n+{\n+\tfor (; options->type != OPTION_END; options++)\n+\t\t;\n+\tif (options->value && options->strdup_fn) {\n+\t\tctx->unknown_opts = options->value;\n+\t\tctx->strdup_fn = options->strdup_fn;\n+\t\treturn;\n+\t}\n+}\n+\n static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n \t\t\t\t  int argc, const char **argv, const char *prefix,\n \t\t\t\t  const struct option *options,\n@@ -655,6 +667,7 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n \tctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n \tctx->flags = flags;\n \tctx->has_subcommands = has_subcommands(options);\n+\tset_strdup_fn(ctx, options);\n \tif (!ctx->has_subcommands && (flags & PARSE_OPT_SUBCOMMAND_OPTIONAL))\n \t\tBUG(\"Using PARSE_OPT_SUBCOMMAND_OPTIONAL without subcommands\");\n \tif (ctx->has_subcommands) {\n@@ -981,7 +994,15 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t *\n \t\t\t\t\t * This is leaky, too bad.\n \t\t\t\t\t */\n-\t\t\t\t\tctx->argv[0] = xstrdup(ctx->opt - 1);\n+\t\t\t\t\tif (ctx->unknown_opts && ctx->strdup_fn) {\n+\t\t\t\t\t\tctx->argv[0] =\n+\t\t\t\t\t\t\tctx->strdup_fn(ctx->unknown_opts,\n+\t\t\t\t\t\t\t\t       ctx->opt -\n+\t\t\t\t\t\t\t\t\t       1);\n+\t\t\t\t\t} else {\n+\t\t\t\t\t\tctx->argv[0] =\n+\t\t\t\t\t\t\txstrdup(ctx->opt - 1);\n+\t\t\t\t\t}\n \t\t\t\t\t*(char *)ctx->argv[0] = '-';\n \t\t\t\t\tgoto unknown;\n \t\t\t\tcase PARSE_OPT_NON_OPTION:\ndiff --git a/parse-options.h b/parse-options.h\nindex 91c3e3c29b3..57037bd381a 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -77,6 +77,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,\n typedef int parse_opt_subcommand_fn(int argc, const char **argv,\n \t\t\t\t    const char *prefix, struct repository *repo);\n \n+typedef char *parse_opt_strdup_fn(void *value, const char *s);\n+\n /*\n  * `type`::\n  *   holds the type of the option, you must have an OPTION_END last in your\n@@ -165,6 +167,7 @@ struct option {\n \tparse_opt_ll_cb *ll_callback;\n \tintptr_t extra;\n \tparse_opt_subcommand_fn *subcommand_fn;\n+\tparse_opt_strdup_fn *strdup_fn;\n };\n \n #define OPT_BIT_F(s, l, v, h, b, f) { \\\n@@ -388,6 +391,13 @@ static char *parse_options_noop_ignored_value MAYBE_UNUSED;\n }\n #define OPT_SUBCOMMAND(l, v, fn)    OPT_SUBCOMMAND_F((l), (v), (fn), 0)\n \n+#define OPT_UNKNOWN(v, fn)          \\\n+\t{                           \\\n+\t\t.type = OPTION_END, \\\n+\t\t.value = (v),       \\\n+\t\t.strdup_fn = (fn),  \\\n+\t}\n+\n /*\n  * parse_options() will filter out the processed options and leave the\n  * non-option arguments in argv[]. argv0 is assumed program name and\n@@ -496,6 +506,8 @@ struct parse_opt_ctx_t {\n \tconst char *prefix;\n \tconst char **alias_groups; /* must be in groups of 3 elements! */\n \tstruct parse_opt_cmdmode_list *cmdmode_list;\n+\tvoid *unknown_opts;\n+\tparse_opt_strdup_fn *strdup_fn;\n };\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\ndiff --git a/t/helper/meson.build b/t/helper/meson.build\nindex d2cabaa2bcf..476e3278176 100644\n--- a/t/helper/meson.build\n+++ b/t/helper/meson.build\n@@ -39,6 +39,7 @@ test_tool_sources = [\n   'test-pack-mtimes.c',\n   'test-parse-options.c',\n   'test-parse-pathspec-file.c',\n+  'test-free-unknown-options.c',\n   'test-partial-clone.c',\n   'test-path-utils.c',\n   'test-path-walk.c',\ndiff --git a/t/helper/test-free-unknown-options.c b/t/helper/test-free-unknown-options.c\nnew file mode 100644\nindex 00000000000..7369dfe379d\n--- /dev/null\n+++ b/t/helper/test-free-unknown-options.c\n@@ -0,0 +1,42 @@\n+#include \"git-compat-util.h\"\n+#include \"parse-options.h\"\n+#include \"setup.h\"\n+#include \"strvec.h\"\n+#include \"test-tool.h\"\n+\n+static const char *const free_unknown_options_usage[] = {\n+\t\"test-tool free-unknown-options\", NULL\n+};\n+\n+static char *strvec_push_wrapper(void *value, const char *str)\n+{\n+\tstruct strvec *sv = value;\n+\treturn (char *)strvec_push(sv, str);\n+}\n+\n+int cmd__free_unknown_options(int argc, const char **argv)\n+{\n+\tstruct strvec *unknown_opts = xmalloc(sizeof(struct strvec));\n+\tconst char *prefix = setup_git_directory();\n+\tint a = 0, b = 0;\n+\tsize_t i;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL('a', \"test-a\", &a, N_(\"option a, only for test use\")),\n+\t\tOPT_BOOL('b', \"test-b\", &b, N_(\"option b, only for test use\")),\n+\t\tOPT_UNKNOWN(unknown_opts, strvec_push_wrapper),\n+\t};\n+\n+\tstrvec_init(unknown_opts);\n+\tparse_options(argc, argv, prefix, options, free_unknown_options_usage,\n+\t\t      PARSE_OPT_KEEP_UNKNOWN_OPT);\n+\n+\tprintf(\"a = %s\\n\", a ? \"true\" : \"false\");\n+\tprintf(\"b = %s\\n\", b ? \"true\" : \"false\");\n+\n+\tfor (i = 0; i < unknown_opts->nr; i++)\n+\t\tprintf(\"free unknown option: %s\\n\", unknown_opts->v[i]);\n+\tstrvec_clear(unknown_opts);\n+\tfree(unknown_opts);\n+\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 50dc4dac4ed..5af5acae1cb 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -51,6 +51,7 @@ static struct test_cmd cmds[] = {\n \t{ \"parse-options-flags\", cmd__parse_options_flags },\n \t{ \"parse-pathspec-file\", cmd__parse_pathspec_file },\n \t{ \"parse-subcommand\", cmd__parse_subcommand },\n+\t{ \"free-unknown-options\", cmd__free_unknown_options },\n \t{ \"partial-clone\", cmd__partial_clone },\n \t{ \"path-utils\", cmd__path_utils },\n \t{ \"path-walk\", cmd__path_walk },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 6d62a5b53d9..33fa7828b9f 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -44,6 +44,7 @@ int cmd__parse_options(int argc, const char **argv);\n int cmd__parse_options_flags(int argc, const char **argv);\n int cmd__parse_pathspec_file(int argc, const char** argv);\n int cmd__parse_subcommand(int argc, const char **argv);\n+int cmd__free_unknown_options(int argc, const char **argv);\n int cmd__partial_clone(int argc, const char **argv);\n int cmd__path_utils(int argc, const char **argv);\n int cmd__path_walk(int argc, const char **argv);\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex ca55ea8228c..df221be5d18 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -822,4 +822,17 @@ test_expect_success 'u16 limits range' '\n \ttest_grep \"value 65536 for option .u16. not in range \\[0,65535\\]\" err\n '\n \n+test_expect_success 'free unknown options' '\n+\ttest-tool free-unknown-options -ac -bd \\\n+\t>output 2>output.err &&\n+\tcat >expect <<-\\EOF &&\n+\ta = true\n+\tb = true\n+\tfree unknown option: -c\n+\tfree unknown option: -d\n+\tEOF\n+\ttest_cmp expect output &&\n+\ttest_must_be_empty output.err\n+'\n+\n test_done\n\nbase-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75\n-- \ngitgitgadget\n"},{"id":"517624","messageId":"aB2ejA1tCK9DR1Nq@pks.im","threadId":"63411","inReplyTo":"pull.1954.v2.git.git.1746624294017.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-09T06:19:56Z","receivedAt":"2025-05-09T06:20:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 07, 2025 at 01:24:53PM +0000, Lidong Yan via GitGitGadget wrote:\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n> \n> Most Git commands use parse_options to parse options, but parse_options\n> causes a memory leak when it encounters unknown short options. The leak\n> occurs at line 984 in parse-options.c, where git allocates some spaces\n\nAgain, no need to quote the exact line number.\n\n> to store the unknown short options and will never release those spaces.\n> \n> To address this issue, users can be allowed to provide a custom function\n> to allocate memory for unknown options. For example, users can use\n> strvec_push to allocate memory for unknown options and later call\n> strvec_clear at the end of the Git command to release the memory, thereby\n> avoiding the memory leak.\n\nSo does that mean that _every_ user of the \"parse-options\" interfaces\nnow has to explicitly plug this memory leak when facing unknown options?\nThat sounds rather undesirable, as there are so many users out there.\n\n> This commit allows users to provide a custom allocation function for\n> unknown options via the strdup_fn field in the last struct option. For\n> convenience, this commit also introduces a new macro, OPT_UNKNOWN, which\n> behaves like OPT_END but takes an additional argument for the allocation\n> function. parse_options could get the custom allocation function in struct\n> parse_opt_ctx_t by using set_strdup_fn. A simple example to use\n> OPT_UNKNOWN is put into t/helper/test-free-unknown-options.c\n\nHm. Is there any other usecase for the `strdup_fn` field that you can\nthink about in the future? Otherwise it feels a bit overengineered from\nmy perspective.\n\nI unfortuntaely don't think the proposed solution works well. To really\nplug the memory leak everywhere, we would have to convert every single\nuser of `parse_options()` to pass `OPTION_UNKNOWN()` instead of\n`OPTION_END()`, pass a strvec and clear that strvec at the end. It just\ndoesn't quite feel like it's worth it to plug a single, bounded memory\nleak.\n\nThe most important problem that we're facing is that the allocated\nstring has a lifetime longer than `parse_options()`, because it really\nonly becomes relevant in the case where `PARSE_OPT_KEEP_UNKNOWN_OPT` is\nenabled. If so, the _caller_ of `parse_options()` will want to process\nthe modified `argv`, and thus they have to be the ones responsible for\nfreeing potentially-allocated memory once it's unused.\n\nBut that'd require us to keep some state. You solve this via the extra\nstrvec, but that doesn't really look like a great solution to me. An\nalternative would be to expose the `parse_options_ctx` and let the\ncaller eventually call `parse_options_clear()` or something like that.\nThis would also require us to modify every caller, but the benefit would\nbe that we now have a single place where we can always allocate memory\nin without having to worry about its lifetime.\n\nAll of that starts to become kind of involved though. So unless others\ndisagree with my analysis I just don't think this edge case really is\nworth the complexity.\n\nThanks!\n\nPatrick\n"},{"id":"517628","messageId":"CC653689-3047-4DFD-9893-46D08F948C10@smail.nju.edu.cn","threadId":"63411","inReplyTo":"aB2ejA1tCK9DR1Nq@pks.im","subject":"Re: [PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-09T07:35:18Z","receivedAt":"2025-05-09T07:35:51Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025年5月9日 14:19，Patrick Steinhardt <ps@pks.im> 写道：\n> So does that mean that _every_ user of the \"parse-options\" interfaces\n> now has to explicitly plug this memory leak when facing unknown options?\n> That sounds rather undesirable, as there are so many users out there.\n\nSince the lifetime of `argv` last until the program terminates, a memory leak\ncan only occur if parse_option is called multiple times and at least two of \nthose calls use the `PARSE_OPT_KEEP_UNKNOWN` flag. In the other\nwords, the memory leak only occurs when the statement `ctx->argue[0] = xstrdup`\noverwrites the result of a previous `xstrdup` call.\n\n> Hm. Is there any other usecase for the `strdup_fn` field that you can\n> think about in the future? Otherwise it feels a bit overengineered from\n> my perspective.\n\nI think a simple approach is to add a marker to the string allocated by `xstrdup`\n, and before the next potential leaking `ctx->argv[0] = xstrdup`, check whether the\nstring needs to be freed. Like we could allocate one more byte in the end of the\nstring to store the marker.\n\n"},{"id":"517662","messageId":"ac89ba0a-b50e-469c-99ce-c70b825ae4af@crinan.ddns.net","threadId":"63411","inReplyTo":"aB2ejA1tCK9DR1Nq@pks.im","subject":"Re: [PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-09T13:08:52Z","receivedAt":"2025-05-09T13:08:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/05/2025 07:19, Patrick Steinhardt wrote:\n> \n> All of that starts to become kind of involved though. So unless others\n> disagree with my analysis I just don't think this edge case really is\n> worth the complexity.\n\nI agree with this. We seem to be copying the string because argv is a\nconst char** but would it really be so bad to modify the string that\ngets passed to us? We know that any bundled options that come before\nan unknown option cannot have a value or else what we see as the\nunknown option would be part of that value. So I think we could do\n\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 35fbb3b0d6..9e6e46da27 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -924,12 +924,12 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n                                          if (internal_help && *ctx->opt == 'h')\n                                                  goto show_usage;\n\n-                                        /* fake a short option thing to hide the fact that we may have\n+                                        /* move a short option thing to hide the fact that we may have\n                                           * started to parse aggregated stuff\n-                                         *\n-                                         * This is leaky, too bad.\n                                           */\n-                                        ctx->argv[0] = xstrdup(ctx->opt - 1);\n+                                        MOVE_ARRAY((char *)arg, ctx->opt - 1,\n+                                                   strlen(ctx->opt) + 2);\n+                                        ctx->argv[0] = arg;\n                                          *(char *)ctx->argv[0] = '-';\n                                          goto unknown;\n                                  case PARSE_OPT_NON_OPTION:\n\nBest Wishes\n\nPhillip\n\n"},{"id":"517663","messageId":"0F0385E0-E5BB-48E0-8E0E-507AB6857CB8@smail.nju.edu.cn","threadId":"63411","inReplyTo":"ac89ba0a-b50e-469c-99ce-c70b825ae4af@crinan.ddns.net","subject":"Re: [PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-09T13:43:14Z","receivedAt":"2025-05-09T13:43:45Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"On 09/05/2025 21:08，Phillip Wood <phillip.wood123@gmail.com> write：\n> diff --git a/parse-options.c b/parse-options.c\n> index 35fbb3b0d6..9e6e46da27 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -924,12 +924,12 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n>                                         if (internal_help && *ctx->opt == 'h')\n>                                                 goto show_usage;\n> \n> -                                        /* fake a short option thing to hide the fact that we may have\n> +                                        /* move a short option thing to hide the fact that we may have\n>                                          * started to parse aggregated stuff\n> -                                         *\n> -                                         * This is leaky, too bad.\n>                                          */\n> -                                        ctx->argv[0] = xstrdup(ctx->opt - 1);\n> +                                        MOVE_ARRAY((char *)arg, ctx->opt - 1,\n> +                                                   strlen(ctx->opt) + 2);\n> +                                        ctx->argv[0] = arg;\n>                                         *(char *)ctx->argv[0] = '-';\n>                                         goto unknown;\n>                                 case PARSE_OPT_NON_OPTION:\n\nI’m not sure why git used to use `xtrdup` here instead of modifying `arg` directly.\nIf modifying `arg` is safe, then this is indeed a good solution."},{"id":"517683","messageId":"pull.1954.v3.git.git.1746800913128.gitgitgadget@gmail.com","threadId":"63411","inReplyTo":"pull.1954.v2.git.git.1746624294017.gitgitgadget@gmail.com","subject":"[PATCH v3] parse-options: fix memory leak when calling `parse_options`","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-09T14:28:32Z","receivedAt":"2025-05-09T14:28:36Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\ncall parse_options twice and occasionally meet unknown option\nin the same place would cause memory leak, since the second `xstrdup`\nin `parse_options_step` would make the first `xstrdup` unreachable.\n\nOne solution is allocate one more magic byte for the unknown option to\nindicate that argv[?] stores a heap allocated arg. Assume for all\narg, `*((char *)arg - 1)` is valid, we could put a magic number before\nthe unknown option. Next time calling \"xstrdup\" will check the magic\nnumber first, and frees the previous heap allocated memory.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    fix xstrdup leak in parse_short_opt\n    \n    Pass a user defined strdup-like function in parse_opt_ctx to avoid\n    memory leak.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1954%2Fbrandb97%2Ffix-parse-option-leak-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1954/brandb97/fix-parse-option-leak-v3\nPull-Request: https://github.com/git/git/pull/1954\n\nRange-diff vs v2:\n\n 1:  e7b4465b83e < -:  ----------- parse-options: fix xstrdup leak in parse_options_step parse-options:984\n -:  ----------- > 1:  475f7b5b1bd parse-options: fix memory leak when calling `parse_options`\n\n\n parse-options.c | 12 +++++++++++-\n parse-options.h |  2 ++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex a9a39ecaef6..84f6ae90c77 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -886,6 +886,8 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t const char * const usagestr[])\n {\n \tint internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);\n+\tchar *magic_ptr = NULL;\n+\tsize_t opt_sz = 0;\n \n \t/* we must reset ->opt, unknown short option leave it dangling */\n \tctx->opt = NULL;\n@@ -981,7 +983,15 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t *\n \t\t\t\t\t * This is leaky, too bad.\n \t\t\t\t\t */\n-\t\t\t\t\tctx->argv[0] = xstrdup(ctx->opt - 1);\n+\t\t\t\t\tmagic_ptr = (char *)ctx->argv[0] - 1;\n+\t\t\t\t\tif (*magic_ptr == OPT_MAGIC)\n+\t\t\t\t\t\tfree(magic_ptr);\n+\t\t\t\t\topt_sz = strlen(ctx->opt - 1) + 1;\n+\t\t\t\t\tmagic_ptr = xmalloc(opt_sz + 1);\n+\t\t\t\t\t*magic_ptr = OPT_MAGIC;\n+\t\t\t\t\tctx->argv[0] = magic_ptr + 1;\n+\t\t\t\t\tmemcpy((char *)ctx->argv[0],\n+\t\t\t\t\t       ctx->opt - 1, opt_sz);\n \t\t\t\t\t*(char *)ctx->argv[0] = '-';\n \t\t\t\t\tgoto unknown;\n \t\t\t\tcase PARSE_OPT_NON_OPTION:\ndiff --git a/parse-options.h b/parse-options.h\nindex 91c3e3c29b3..7fdd2e1097a 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -477,6 +477,8 @@ static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,\n \t\tBUG(\"option callback expects an argument\"); \\\n } while(0)\n \n+#define OPT_MAGIC ((char)(0xee))\n+\n /*----- incremental advanced APIs -----*/\n \n struct parse_opt_cmdmode_list;\n\nbase-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75\n-- \ngitgitgadget\n"}]}