* [PATCH 1/3] fix xstrdup leak in parse_short_opt
2025-05-07 2:33 [PATCH 0/3] fix xstrdup leak in parse_short_opt Lidong Yan via GitGitGadget
@ 2025-05-07 2:33 ` Lidong Yan via GitGitGadget
2025-05-07 7:54 ` Patrick Steinhardt
2025-05-07 2:33 ` [PATCH 2/3] fix: replace bug where int was incorrectly used as bool Lidong Yan via GitGitGadget
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Lidong Yan via GitGitGadget @ 2025-05-07 2:33 UTC (permalink / raw)
To: git; +Cc: Lidong Yan, Lidong Yan
From: Lidong Yan <502024330056@smail.nju.edu.cn>
Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
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
diff --git a/Makefile b/Makefile
index 8a7f1c76543f..af8ea677b820 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/parse-options.c b/parse-options.c
index a9a39ecaef6c..4279dfe4d313 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -638,6 +638,16 @@ static int has_subcommands(const struct option *options)
return 0;
}
+static void set_strdup_fn(struct parse_opt_ctx_t *ctx, const struct option *options) {
+ 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:
diff --git a/parse-options.h b/parse-options.h
index 91c3e3c29b3d..af06a09abb8e 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -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,
diff --git a/t/helper/meson.build b/t/helper/meson.build
index d2cabaa2bcfc..476e32781761 100644
--- a/t/helper/meson.build
+++ b/t/helper/meson.build
@@ -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',
diff --git a/t/helper/test-free-unknown-options.c b/t/helper/test-free-unknown-options.c
new file mode 100644
index 000000000000..9d658115ba8f
--- /dev/null
+++ b/t/helper/test-free-unknown-options.c
@@ -0,0 +1,35 @@
+#include "git-compat-util.h"
+#include "parse-options.h"
+#include "setup.h"
+#include "strvec.h"
+
+static const char *const free_unknown_options_usage[] = {
+ "test-tool free-unknown-options",
+ NULL
+};
+
+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
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 50dc4dac4ed6..79ec4f9cda07 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -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 },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 6d62a5b53d95..33fa7828b9f6 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -44,6 +44,7 @@ int cmd__parse_options(int argc, const char **argv);
int cmd__parse_options_flags(int argc, const char **argv);
int cmd__parse_pathspec_file(int argc, const char** argv);
int cmd__parse_subcommand(int argc, const char **argv);
+int cmd__free_unknown_options(int argc, const char **argv);
int cmd__partial_clone(int argc, const char **argv);
int cmd__path_utils(int argc, const char **argv);
int cmd__path_walk(int argc, const char **argv);
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ca55ea8228c3..773f54103fd3 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -822,4 +822,18 @@ test_expect_success 'u16 limits range' '
test_grep "value 65536 for option .u16. not in range \[0,65535\]" err
'
+cat >expect <<\EOF
+a = true
+b = true
+free unknown option: -c
+free unknown option: -d
+EOF
+
+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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] fix xstrdup leak in parse_short_opt
2025-05-07 2:33 ` [PATCH 1/3] " Lidong Yan via GitGitGadget
@ 2025-05-07 7:54 ` Patrick Steinhardt
0 siblings, 0 replies; 14+ messages in thread
From: Patrick Steinhardt @ 2025-05-07 7:54 UTC (permalink / raw)
To: Lidong Yan via GitGitGadget; +Cc: git, Lidong Yan
On Wed, May 07, 2025 at 02:33:21AM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <502024330056@smail.nju.edu.cn>
This commit is missing an explanation of the problem. When does it
trigger? Which subsystem triggers it? Why didn't we observe the issue
beforehand? And given that the solution seems to be a bit more complex
it should also explain how you're fixing the issue.
Furthermore, the subject of such a commit should be prefixed with the
subsystem in which you're fixing the issue. So in your case,
"parse-options:".
I'd recommend reading through "Documentation/MyFirstContribution.adoc",
which explains all of this.
> diff --git a/parse-options.c b/parse-options.c
> index a9a39ecaef6c..4279dfe4d313 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -638,6 +638,16 @@ static int has_subcommands(const struct option *options)
> return 0;
> }
>
> +static void set_strdup_fn(struct parse_opt_ctx_t *ctx, const struct option *options) {
Style: the opening brace should be on its own line.
> + for (; options->type != OPTION_END; options++)
> + ;
> + if (options->value && options->strdup_fn) {
> + ctx->unknown_opts = options->value;
> + ctx->strdup_fn = options->strdup_fn;
> + return;
> + }
It's not really clear what this is doing. Is the intent to only change
this this value for the last option? If so, why?
> @@ -981,7 +992,11 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
> *
> * This is leaky, too bad.
> */
> - ctx->argv[0] = xstrdup(ctx->opt - 1);
> + if (ctx->unknown_opts && ctx->strdup_fn) {
> + ctx->argv[0] = ctx->strdup_fn(ctx->unknown_opts, ctx->opt - 1);
> + } else {
> + ctx->argv[0] = xstrdup(ctx->opt - 1);
> + }
> *(char *)ctx->argv[0] = '-';
> goto unknown;
> case PARSE_OPT_NON_OPTION:
Hm. I'm at a loss what we're solving here. All of this really should be
explained in the commit message to give context to the reviewer.
> diff --git a/parse-options.h b/parse-options.h
> index 91c3e3c29b3d..af06a09abb8e 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -388,6 +391,12 @@ static char *parse_options_noop_ignored_value MAYBE_UNUSED;
> }
> #define OPT_SUBCOMMAND(l, v, fn) OPT_SUBCOMMAND_F((l), (v), (fn), 0)
>
> +#define OPT_UNKNOWN(v, fn) { \
> + .type = OPTION_END, \
> + .value = (v), \
> + .strdup_fn = (fn), \
> +}
> +
> /*
> * parse_options() will filter out the processed options and leave the
> * non-option arguments in argv[]. argv0 is assumed program name and
> @@ -496,6 +505,9 @@ struct parse_opt_ctx_t {
> const char *prefix;
> const char **alias_groups; /* must be in groups of 3 elements! */
> struct parse_opt_cmdmode_list *cmdmode_list;
> +
> + void *unknown_opts;
> + parse_opt_strdup_fn *strdup_fn;
> };
>
> void parse_options_start(struct parse_opt_ctx_t *ctx,
Okay. So the intent seems to be that we start storing any options that
we don't understand?
> diff --git a/t/helper/test-free-unknown-options.c b/t/helper/test-free-unknown-options.c
> new file mode 100644
> index 000000000000..9d658115ba8f
> --- /dev/null
> +++ b/t/helper/test-free-unknown-options.c
> @@ -0,0 +1,35 @@
> +#include "git-compat-util.h"
> +#include "parse-options.h"
> +#include "setup.h"
> +#include "strvec.h"
> +
> +static const char *const free_unknown_options_usage[] = {
> + "test-tool free-unknown-options",
> + NULL
> +};
Can't we expand `test-tool parse-options` instead of introducing a new
helper?
> +int cmd__free_unknown_options(int argc, const char **argv) {
> + struct strvec *unknown_opts = xmalloc(sizeof(struct strvec));
> + strvec_init(unknown_opts);
> + const char *prefix = setup_git_directory();
> +
> + bool a, b;
> + struct option options[] = {
> + OPT_BOOL('a', "test-a", &a, N_("option a, only for test use")),
> + OPT_BOOL('b', "test-b", &b, N_("option b, only for test use")),
> + OPT_UNKNOWN(unknown_opts, (parse_opt_strdup_fn *)&strvec_push),
> + };
> +
> + parse_options(argc, argv, prefix, options,
> + free_unknown_options_usage, PARSE_OPT_KEEP_UNKNOWN_OPT);
> +
> + printf("a = %s\n", a? "true": "false");
> + printf("b = %s\n", b? "true": "false");
> +
> + int i;
> + for (i = 0; i < unknown_opts->nr; i++) {
> + printf("free unknown option: %s\n", unknown_opts->v[i]);
> + }
> + strvec_clear(unknown_opts);
> + free(unknown_opts);
> +}
> \ No newline at end of file
Missing newline.
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 6d62a5b53d95..33fa7828b9f6 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -44,6 +44,7 @@ int cmd__parse_options(int argc, const char **argv);
> int cmd__parse_options_flags(int argc, const char **argv);
> int cmd__parse_pathspec_file(int argc, const char** argv);
> int cmd__parse_subcommand(int argc, const char **argv);
> +int cmd__free_unknown_options(int argc, const char **argv);
> int cmd__partial_clone(int argc, const char **argv);
> int cmd__path_utils(int argc, const char **argv);
> int cmd__path_walk(int argc, const char **argv);
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index ca55ea8228c3..773f54103fd3 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -822,4 +822,18 @@ test_expect_success 'u16 limits range' '
> test_grep "value 65536 for option .u16. not in range \[0,65535\]" err
> '
>
> +cat >expect <<\EOF
> +a = true
> +b = true
> +free unknown option: -c
> +free unknown option: -d
> +EOF
This should be done inside `test_expect_success` itself.
> +test_expect_success 'free unknown options' '
> + test-tool free-unknown-options -ac -bd \
> + >output 2>output.err &&
> + test_cmp expect output &&
> + test_must_be_empty output.err
> +'
> +
> test_done
Okay, so the memory leak seems to happen when handling unknown options
indeed, as I started to expect after the middle of this patch. In any
case, this commit really needs to be polished to have a proper commit
message that sets the stage for the reviewer.
Patrick
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] fix: replace bug where int was incorrectly used as bool
2025-05-07 2:33 [PATCH 0/3] fix xstrdup leak in parse_short_opt Lidong Yan via GitGitGadget
2025-05-07 2:33 ` [PATCH 1/3] " Lidong Yan via GitGitGadget
@ 2025-05-07 2:33 ` Lidong Yan via GitGitGadget
2025-05-07 7:54 ` Patrick Steinhardt
2025-05-07 2:33 ` [PATCH 3/3] fix: use strvec_push_wrapper to prevent ubsan failure Lidong Yan via GitGitGadget
2025-05-07 13:24 ` [PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984 Lidong Yan via GitGitGadget
3 siblings, 1 reply; 14+ messages in thread
From: Lidong Yan via GitGitGadget @ 2025-05-07 2:33 UTC (permalink / raw)
To: git; +Cc: Lidong Yan, Lidong Yan
From: Lidong Yan <502024330056@smail.nju.edu.cn>
Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
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(-)
diff --git a/parse-options.c b/parse-options.c
index 4279dfe4d313..12721b83000a 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -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;
diff --git a/parse-options.h b/parse-options.h
index af06a09abb8e..57037bd381a3 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -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;
};
diff --git a/t/helper/test-free-unknown-options.c b/t/helper/test-free-unknown-options.c
index 9d658115ba8f..59d732da23ca 100644
--- a/t/helper/test-free-unknown-options.c
+++ b/t/helper/test-free-unknown-options.c
@@ -2,34 +2,35 @@
#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();
-
- 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, (parse_opt_strdup_fn *)&strvec_push),
};
- 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");
+ 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);
- 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
+ return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 79ec4f9cda07..5af5acae1cb3 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] fix: replace bug where int was incorrectly used as bool
2025-05-07 2:33 ` [PATCH 2/3] fix: replace bug where int was incorrectly used as bool Lidong Yan via GitGitGadget
@ 2025-05-07 7:54 ` Patrick Steinhardt
0 siblings, 0 replies; 14+ messages in thread
From: Patrick Steinhardt @ 2025-05-07 7:54 UTC (permalink / raw)
To: Lidong Yan via GitGitGadget; +Cc: git, Lidong Yan
On Wed, May 07, 2025 at 02:33:22AM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>
> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
The same is true here -- this needs to have a proper commit subject and
message.
> diff --git a/parse-options.c b/parse-options.c
> index 4279dfe4d313..12721b83000a 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -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) {
Please refrain from doing code style cleanups of code you have just
introduced in the preceding commit.
Patrick
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] fix: use strvec_push_wrapper to prevent ubsan failure
2025-05-07 2:33 [PATCH 0/3] fix xstrdup leak in parse_short_opt Lidong Yan via GitGitGadget
2025-05-07 2:33 ` [PATCH 1/3] " Lidong Yan via GitGitGadget
2025-05-07 2:33 ` [PATCH 2/3] fix: replace bug where int was incorrectly used as bool Lidong Yan via GitGitGadget
@ 2025-05-07 2:33 ` Lidong Yan via GitGitGadget
2025-05-07 7:54 ` Patrick Steinhardt
2025-05-07 13:24 ` [PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984 Lidong Yan via GitGitGadget
3 siblings, 1 reply; 14+ messages in thread
From: Lidong Yan via GitGitGadget @ 2025-05-07 2:33 UTC (permalink / raw)
To: git; +Cc: Lidong Yan, Lidong Yan
From: Lidong Yan <502024330056@smail.nju.edu.cn>
Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
t/helper/test-free-unknown-options.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/t/helper/test-free-unknown-options.c b/t/helper/test-free-unknown-options.c
index 59d732da23ca..7369dfe379d6 100644
--- a/t/helper/test-free-unknown-options.c
+++ b/t/helper/test-free-unknown-options.c
@@ -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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 3/3] fix: use strvec_push_wrapper to prevent ubsan failure
2025-05-07 2:33 ` [PATCH 3/3] fix: use strvec_push_wrapper to prevent ubsan failure Lidong Yan via GitGitGadget
@ 2025-05-07 7:54 ` Patrick Steinhardt
2025-05-07 12:19 ` lidongyan
0 siblings, 1 reply; 14+ messages in thread
From: Patrick Steinhardt @ 2025-05-07 7:54 UTC (permalink / raw)
To: Lidong Yan via GitGitGadget; +Cc: git, Lidong Yan
On Wed, May 07, 2025 at 02:33:23AM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>
> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] fix: use strvec_push_wrapper to prevent ubsan failure
2025-05-07 7:54 ` Patrick Steinhardt
@ 2025-05-07 12:19 ` lidongyan
0 siblings, 0 replies; 14+ messages in thread
From: lidongyan @ 2025-05-07 12:19 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Lidong Yan via GitGitGadget, git
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.
> 2025年5月7日 15:54，Patrick Steinhardt <ps@pks.im> 写道：
>
> On Wed, May 07, 2025 at 02:33:23AM +0000, Lidong Yan via GitGitGadget wrote:
>> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>>
>> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
>
> 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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984
2025-05-07 2:33 [PATCH 0/3] fix xstrdup leak in parse_short_opt Lidong Yan via GitGitGadget
` (2 preceding siblings ...)
2025-05-07 2:33 ` [PATCH 3/3] fix: use strvec_push_wrapper to prevent ubsan failure Lidong Yan via GitGitGadget
@ 2025-05-07 13:24 ` Lidong Yan via GitGitGadget
2025-05-09 6:19 ` Patrick Steinhardt
2025-05-09 14:28 ` [PATCH v3] parse-options: fix memory leak when calling `parse_options` Lidong Yan via GitGitGadget
3 siblings, 2 replies; 14+ messages in thread
From: Lidong Yan via GitGitGadget @ 2025-05-07 13:24 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Lidong Yan, Lidong Yan
From: Lidong Yan <502024330056@smail.nju.edu.cn>
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 <502024330056@smail.nju.edu.cn>
---
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 <502024330056@smail.nju.edu.cn>
## 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 <502024330056@smail.nju.edu.cn>
@@ 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
diff --git a/Makefile b/Makefile
index 8a7f1c76543..af8ea677b82 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/parse-options.c b/parse-options.c
index a9a39ecaef6..12721b83000 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -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:
diff --git a/parse-options.h b/parse-options.h
index 91c3e3c29b3..57037bd381a 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -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,
diff --git a/t/helper/meson.build b/t/helper/meson.build
index d2cabaa2bcf..476e3278176 100644
--- a/t/helper/meson.build
+++ b/t/helper/meson.build
@@ -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',
diff --git a/t/helper/test-free-unknown-options.c b/t/helper/test-free-unknown-options.c
new file mode 100644
index 00000000000..7369dfe379d
--- /dev/null
+++ b/t/helper/test-free-unknown-options.c
@@ -0,0 +1,42 @@
+#include "git-compat-util.h"
+#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
+};
+
+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));
+ 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, strvec_push_wrapper),
+ };
+
+ 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");
+
+ 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;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 50dc4dac4ed..5af5acae1cb 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -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 },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 6d62a5b53d9..33fa7828b9f 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -44,6 +44,7 @@ int cmd__parse_options(int argc, const char **argv);
int cmd__parse_options_flags(int argc, const char **argv);
int cmd__parse_pathspec_file(int argc, const char** argv);
int cmd__parse_subcommand(int argc, const char **argv);
+int cmd__free_unknown_options(int argc, const char **argv);
int cmd__partial_clone(int argc, const char **argv);
int cmd__path_utils(int argc, const char **argv);
int cmd__path_walk(int argc, const char **argv);
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ca55ea8228c..df221be5d18 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984
2025-05-07 13:24 ` [PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984 Lidong Yan via GitGitGadget
@ 2025-05-09 6:19 ` Patrick Steinhardt
2025-05-09 7:35 ` lidongyan
2025-05-09 13:08 ` Phillip Wood
2025-05-09 14:28 ` [PATCH v3] parse-options: fix memory leak when calling `parse_options` Lidong Yan via GitGitGadget
1 sibling, 2 replies; 14+ messages in thread
From: Patrick Steinhardt @ 2025-05-09 6:19 UTC (permalink / raw)
To: Lidong Yan via GitGitGadget; +Cc: git, Lidong Yan
On Wed, May 07, 2025 at 01:24:53PM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>
> 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
Again, no need to quote the exact line number.
> 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.
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.
> 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
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984
2025-05-09 6:19 ` Patrick Steinhardt
@ 2025-05-09 7:35 ` lidongyan
2025-05-09 13:08 ` Phillip Wood
1 sibling, 0 replies; 14+ messages in thread
From: lidongyan @ 2025-05-09 7:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Lidong Yan via GitGitGadget, git
2025年5月9日 14:19，Patrick Steinhardt <ps@pks.im> 写道：
> 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.
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.
> 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 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984
2025-05-09 6:19 ` Patrick Steinhardt
2025-05-09 7:35 ` lidongyan
@ 2025-05-09 13:08 ` Phillip Wood
2025-05-09 13:43 ` lidongyan
1 sibling, 1 reply; 14+ messages in thread
From: Phillip Wood @ 2025-05-09 13:08 UTC (permalink / raw)
To: Patrick Steinhardt, Lidong Yan via GitGitGadget; +Cc: git, Lidong Yan
On 09/05/2025 07:19, Patrick Steinhardt wrote:
>
> 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.
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
diff --git a/parse-options.c b/parse-options.c
index 35fbb3b0d6..9e6e46da27 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984
2025-05-09 13:08 ` Phillip Wood
@ 2025-05-09 13:43 ` lidongyan
0 siblings, 0 replies; 14+ messages in thread
From: lidongyan @ 2025-05-09 13:43 UTC (permalink / raw)
To: Phillip Wood; +Cc: Patrick Steinhardt, Lidong Yan via GitGitGadget, git
On 09/05/2025 21:08，Phillip Wood <phillip.wood123@gmail.com> write：
> diff --git a/parse-options.c b/parse-options.c
> index 35fbb3b0d6..9e6e46da27 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -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:
I’m not sure why git used to use `xtrdup` here instead of modifying `arg` directly.
If modifying `arg` is safe, then this is indeed a good solution.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3] parse-options: fix memory leak when calling `parse_options`
2025-05-07 13:24 ` [PATCH v2] parse-options: fix xstrdup leak in parse_options_step parse-options:984 Lidong Yan via GitGitGadget
2025-05-09 6:19 ` Patrick Steinhardt
@ 2025-05-09 14:28 ` Lidong Yan via GitGitGadget
1 sibling, 0 replies; 14+ messages in thread
From: Lidong Yan via GitGitGadget @ 2025-05-09 14:28 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Phillip Wood, Lidong Yan, Lidong Yan
From: Lidong Yan <502024330056@smail.nju.edu.cn>
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.
Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
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(-)
diff --git a/parse-options.c b/parse-options.c
index a9a39ecaef6..84f6ae90c77 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -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:
diff --git a/parse-options.h b/parse-options.h
index 91c3e3c29b3..7fdd2e1097a 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -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
^ permalink raw reply related [flat|nested] 14+ messages in thread