diff mbox series

[02/29] parse-options: fix leaks for users of OPT_FILENAME

Message ID ecabbb74e19ea773fd23719bd7fd4937b29679e5.1717402439.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series Memory leak fixes (pt.2) | expand

Commit Message

Patrick Steinhardt June 3, 2024, 9:46 a.m. UTC
The `OPT_FILENAME()` option will, if set, put an allocated string into
the user-provided variable. Consequently, that variable thus needs to be
free'd by the caller of `parse_options()`. Some callsites don't though
and thus leak memory. Fix those.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 apply.c                             |  1 +
 apply.h                             |  2 +-
 builtin/archive.c                   |  7 +++++--
 builtin/commit.c                    |  7 +++++--
 builtin/fmt-merge-msg.c             |  4 +++-
 builtin/log.c                       |  4 +++-
 builtin/multi-pack-index.c          | 13 +++++++++----
 builtin/sparse-checkout.c           |  1 +
 t/helper/test-parse-options.c       |  1 +
 t/t1512-rev-parse-disambiguation.sh |  1 +
 t/t2500-untracked-overwriting.sh    |  1 +
 t/t3406-rebase-message.sh           |  1 +
 t/t3407-rebase-abort.sh             |  1 +
 t/t3428-rebase-signoff.sh           |  1 +
 t/t4131-apply-fake-ancestor.sh      |  1 +
 t/t4151-am-abort.sh                 |  1 +
 t/t4253-am-keep-cr-dos.sh           |  1 +
 t/t4255-am-submodule.sh             |  1 +
 t/t5407-post-rewrite-hook.sh        |  1 +
 t/t6427-diff3-conflict-markers.sh   |  1 +
 t/t7512-status-help.sh              |  1 +
 21 files changed, 41 insertions(+), 11 deletions(-)

Comments

karthik nayak June 6, 2024, 10 a.m. UTC | #1
Patrick Steinhardt <ps@pks.im> writes:

[snip]

> diff --git a/builtin/archive.c b/builtin/archive.c
> index 15ee1ec7bb..f29c0ef6ad 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -92,6 +92,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
>  			N_("path to the remote git-upload-archive command")),
>  		OPT_END()
>  	};
> +	int ret;
>
>  	argc = parse_options(argc, argv, prefix, local_opts, NULL,
>  			     PARSE_OPT_KEEP_ALL);
> @@ -106,6 +107,8 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
>
>  	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
>
> -	UNLEAK(output);
> -	return write_archive(argc, argv, prefix, the_repository, output, 0);
> +	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
> +
> +	free(output);
> +	return ret;

Since output is passed to `write_archive`, we capture the return and
free output. Makes sense.

[snip]

> diff --git a/builtin/log.c b/builtin/log.c
> index 78a247d8a9..4e4b645a21 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -2021,7 +2021,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	const char *rfc = NULL;
>  	int creation_factor = -1;
>  	const char *signature = git_version_string;
> -	const char *signature_file_arg = NULL;
> +	char *signature_file_arg = NULL;
>  	struct keep_callback_data keep_callback_data = {
>  		.cfg = &cfg,
>  		.revs = &rev,
> @@ -2559,6 +2559,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	strbuf_release(&rdiff1);
>  	strbuf_release(&rdiff2);
>  	strbuf_release(&rdiff_title);
> +	free(description_file);
> +	free(signature_file_arg);
>  	free(to_free);
>  	free(rev.message_id);
>  	if (rev.ref_message_ids)
>

I see `char *description_file = NULL` already and it is also used
`OPT_FILENAME`. So all good here.

The rest of the patch looks good too. This was commit was quite easy to
ready. Appreciate the neat breakdown.
diff mbox series

Patch

diff --git a/apply.c b/apply.c
index 901b67e625..d8d26a48f1 100644
--- a/apply.c
+++ b/apply.c
@@ -135,6 +135,7 @@  void clear_apply_state(struct apply_state *state)
 	strset_clear(&state->removed_symlinks);
 	strset_clear(&state->kept_symlinks);
 	strbuf_release(&state->root);
+	FREE_AND_NULL(state->fake_ancestor);
 
 	/* &state->fn_table is cleared at the end of apply_patch() */
 }
diff --git a/apply.h b/apply.h
index 7cd38b1443..36d7c3f70b 100644
--- a/apply.h
+++ b/apply.h
@@ -59,7 +59,7 @@  struct apply_state {
 	struct repository *repo;
 	const char *index_file;
 	enum apply_verbosity apply_verbosity;
-	const char *fake_ancestor;
+	char *fake_ancestor;
 	const char *patch_input_file;
 	int line_termination;
 	struct strbuf root;
diff --git a/builtin/archive.c b/builtin/archive.c
index 15ee1ec7bb..f29c0ef6ad 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -92,6 +92,7 @@  int cmd_archive(int argc, const char **argv, const char *prefix)
 			N_("path to the remote git-upload-archive command")),
 		OPT_END()
 	};
+	int ret;
 
 	argc = parse_options(argc, argv, prefix, local_opts, NULL,
 			     PARSE_OPT_KEEP_ALL);
@@ -106,6 +107,8 @@  int cmd_archive(int argc, const char **argv, const char *prefix)
 
 	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
-	UNLEAK(output);
-	return write_archive(argc, argv, prefix, the_repository, output, 0);
+	ret = write_archive(argc, argv, prefix, the_repository, output, 0);
+
+	free(output);
+	return ret;
 }
diff --git a/builtin/commit.c b/builtin/commit.c
index f53e7e86ff..dcaf4efa03 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -106,7 +106,8 @@  static enum {
 	COMMIT_PARTIAL
 } commit_style;
 
-static const char *logfile, *force_author;
+static const char *force_author;
+static char *logfile;
 static char *template_file;
 /*
  * The _message variables are commit names from which to take
@@ -1309,7 +1310,7 @@  static int parse_and_validate_options(int argc, const char *argv[],
 				  !!use_message, "-C",
 				  !!logfile, "-F");
 	if (use_message || edit_message || logfile ||fixup_message || have_option_m)
-		template_file = NULL;
+		FREE_AND_NULL(template_file);
 	if (edit_message)
 		use_message = edit_message;
 	if (amend && !use_message && !fixup_message)
@@ -1892,5 +1893,7 @@  int cmd_commit(int argc, const char **argv, const char *prefix)
 	strbuf_release(&author_ident);
 	strbuf_release(&err);
 	strbuf_release(&sb);
+	free(logfile);
+	free(template_file);
 	return ret;
 }
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 0f9855b680..957786d1b3 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -11,7 +11,7 @@  static const char * const fmt_merge_msg_usage[] = {
 
 int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 {
-	const char *inpath = NULL;
+	char *inpath = NULL;
 	const char *message = NULL;
 	char *into_name = NULL;
 	int shortlog_len = -1;
@@ -66,5 +66,7 @@  int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 	if (ret)
 		return ret;
 	write_in_full(STDOUT_FILENO, output.buf, output.len);
+
+	free(inpath);
 	return 0;
 }
diff --git a/builtin/log.c b/builtin/log.c
index 78a247d8a9..4e4b645a21 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2021,7 +2021,7 @@  int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	const char *rfc = NULL;
 	int creation_factor = -1;
 	const char *signature = git_version_string;
-	const char *signature_file_arg = NULL;
+	char *signature_file_arg = NULL;
 	struct keep_callback_data keep_callback_data = {
 		.cfg = &cfg,
 		.revs = &rev,
@@ -2559,6 +2559,8 @@  int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	strbuf_release(&rdiff1);
 	strbuf_release(&rdiff2);
 	strbuf_release(&rdiff_title);
+	free(description_file);
+	free(signature_file_arg);
 	free(to_free);
 	free(rev.message_id);
 	if (rev.ref_message_ids)
diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 8360932d2e..9cf1a32d65 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -50,7 +50,7 @@  static char const * const builtin_multi_pack_index_usage[] = {
 static struct opts_multi_pack_index {
 	char *object_dir;
 	const char *preferred_pack;
-	const char *refs_snapshot;
+	char *refs_snapshot;
 	unsigned long batch_size;
 	unsigned flags;
 	int stdin_packs;
@@ -135,6 +135,7 @@  static int cmd_multi_pack_index_write(int argc, const char **argv,
 			     N_("refs snapshot for selecting bitmap commits")),
 		OPT_END(),
 	};
+	int ret;
 
 	opts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;
 
@@ -157,7 +158,6 @@  static int cmd_multi_pack_index_write(int argc, const char **argv,
 
 	if (opts.stdin_packs) {
 		struct string_list packs = STRING_LIST_INIT_DUP;
-		int ret;
 
 		read_packs_from_stdin(&packs);
 
@@ -166,12 +166,17 @@  static int cmd_multi_pack_index_write(int argc, const char **argv,
 					   opts.refs_snapshot, opts.flags);
 
 		string_list_clear(&packs, 0);
+		free(opts.refs_snapshot);
 
 		return ret;
 
 	}
-	return write_midx_file(opts.object_dir, opts.preferred_pack,
-			       opts.refs_snapshot, opts.flags);
+
+	ret = write_midx_file(opts.object_dir, opts.preferred_pack,
+			      opts.refs_snapshot, opts.flags);
+
+	free(opts.refs_snapshot);
+	return ret;
 }
 
 static int cmd_multi_pack_index_verify(int argc, const char **argv,
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 0f52e25249..84a6adf681 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -1011,6 +1011,7 @@  static int sparse_checkout_check_rules(int argc, const char **argv, const char *
 
 	ret = check_rules(&pl, check_rules_opts.null_termination);
 	clear_pattern_list(&pl);
+	free(check_rules_opts.rules_file);
 	return ret;
 }
 
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index ded8116cc5..5250913d99 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -207,6 +207,7 @@  int cmd__parse_options(int argc, const char **argv)
 	expect.strdup_strings = 1;
 	string_list_clear(&expect, 0);
 	string_list_clear(&list, 0);
+	free(file);
 
 	return ret;
 }
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 70f1e0a998..f9d68ce74e 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -23,6 +23,7 @@  one tagged as v1.0.0.  They all have one regular file each.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_cmp_failed_rev_parse () {
diff --git a/t/t2500-untracked-overwriting.sh b/t/t2500-untracked-overwriting.sh
index 5c0bf4d21f..714feb83be 100755
--- a/t/t2500-untracked-overwriting.sh
+++ b/t/t2500-untracked-overwriting.sh
@@ -2,6 +2,7 @@ 
 
 test_description='Test handling of overwriting untracked files'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_setup_reset () {
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index a1d7fa7f7c..82108b67e6 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -5,6 +5,7 @@  test_description='messages from rebase operation'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 9f49c4228b..2c3f38d45a 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -5,6 +5,7 @@  test_description='git rebase --abort tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
index 6f57aed9fa..365436ebfc 100755
--- a/t/t3428-rebase-signoff.sh
+++ b/t/t3428-rebase-signoff.sh
@@ -5,6 +5,7 @@  test_description='git rebase --signoff
 This test runs git rebase --signoff and make sure that it works.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
diff --git a/t/t4131-apply-fake-ancestor.sh b/t/t4131-apply-fake-ancestor.sh
index b1361ce546..40c92115a6 100755
--- a/t/t4131-apply-fake-ancestor.sh
+++ b/t/t4131-apply-fake-ancestor.sh
@@ -5,6 +5,7 @@ 
 
 test_description='git apply --build-fake-ancestor handling.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index edb38da701..1825a89d6a 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -2,6 +2,7 @@ 
 
 test_description='am --abort'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t4253-am-keep-cr-dos.sh b/t/t4253-am-keep-cr-dos.sh
index 0ee69d2a0c..2bcdd9f34f 100755
--- a/t/t4253-am-keep-cr-dos.sh
+++ b/t/t4253-am-keep-cr-dos.sh
@@ -9,6 +9,7 @@  test_description='git-am mbox with dos line ending.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Three patches which will be added as files with dos line ending.
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index a7ba08f728..04f3ccfc41 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -2,6 +2,7 @@ 
 
 test_description='git am handling submodules'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index ad7f8c6f00..e99e728236 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -7,6 +7,7 @@  test_description='Test the post-rewrite hook.'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t6427-diff3-conflict-markers.sh b/t/t6427-diff3-conflict-markers.sh
index dd5fe6a402..a13271b349 100755
--- a/t/t6427-diff3-conflict-markers.sh
+++ b/t/t6427-diff3-conflict-markers.sh
@@ -5,6 +5,7 @@  test_description='recursive merge diff3 style conflict markers'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Setup:
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index 802f8f704c..cdd5f2c697 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -10,6 +10,7 @@  test_description='git status advice'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh