git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: "Eric Sunshine" <sunshine@sunshineco.com>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Andrzej Hunt" <andrzej@ahunt.org>,
	"Andrzej Hunt" <ajrhunt@google.com>
Subject: [PATCH v3 8/9] parse-options: don't leak alias help messages
Date: Sun, 21 Mar 2021 16:58:36 +0000	[thread overview]
Message-ID: <6e46cd332023a579ab0b4c682111c085a634dafb.1616345918.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.899.v3.git.1616345918.gitgitgadget@gmail.com>

From: Andrzej Hunt <ajrhunt@google.com>

preprocess_options() allocates new strings for help messages for
OPTION_ALIAS. Therefore we also need to clean those help messages up
when freeing the returned options.

First introduced in:
  7c280589cf (parse-options: teach "git cmd -h" to show alias as alias, 2020-03-16)

The preprocessed options themselves no longer contain any indication
that a given option is/was an alias - therefore we add a new flag to
indicate former aliases. (An alternative approach would be to look back
at the original options to determine which options are aliases - but
that seems like a fragile approach. Or we could even look at the
alias_groups list - which might be less fragile, but would be slower
as it requires nested looping.)

As far as I can tell, parse_options() is only ever used once per
command, and the help messages are small - hence this leak has very
little impact.

This leak was found while running t0001. LSAN output can be found below:

Direct leak of 65 byte(s) in 1 object(s) allocated from:
    #0 0x49a859 in realloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0x9aae36 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
    #2 0x939d8d in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
    #3 0x93b936 in strbuf_vaddf /home/ahunt/oss-fuzz/git/strbuf.c:392:3
    #4 0x93b7ff in strbuf_addf /home/ahunt/oss-fuzz/git/strbuf.c:333:2
    #5 0x86747e in preprocess_options /home/ahunt/oss-fuzz/git/parse-options.c:666:3
    #6 0x866ed2 in parse_options /home/ahunt/oss-fuzz/git/parse-options.c:847:17
    #7 0x51c4a7 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:989:9
    #8 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #9 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #10 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #11 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #12 0x69c9fe in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #13 0x7fdac42d4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
---
 parse-options.c | 19 ++++++++++++++++++-
 parse-options.h |  1 +
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/parse-options.c b/parse-options.c
index fbea16eaf5c2..e6f56768ca5d 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -625,6 +625,8 @@ static int show_gitcomp(const struct option *opts, int show_all)
  *
  * Right now this is only used to preprocess and substitute
  * OPTION_ALIAS.
+ *
+ * The returned options should be freed using free_preprocessed_options.
  */
 static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 					 const struct option *options)
@@ -678,6 +680,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 			newopt[i].short_name = short_name;
 			newopt[i].long_name = long_name;
 			newopt[i].help = strbuf_detach(&help, NULL);
+			newopt[i].flags |= PARSE_OPT_FROM_ALIAS;
 			break;
 		}
 
@@ -693,6 +696,20 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 	return newopt;
 }
 
+static void free_preprocessed_options(struct option *options)
+{
+	int i;
+
+	if (!options)
+		return;
+
+	for (i = 0; options[i].type != OPTION_END; i++) {
+		if (options[i].flags & PARSE_OPT_FROM_ALIAS)
+			free((void *)options[i].help);
+	}
+	free(options);
+}
+
 static int usage_with_options_internal(struct parse_opt_ctx_t *,
 				       const char * const *,
 				       const struct option *, int, int);
@@ -870,7 +887,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	}
 
 	precompose_argv_prefix(argc, argv, NULL);
-	free(real_options);
+	free_preprocessed_options(real_options);
 	free(ctx.alias_groups);
 	return parse_options_end(&ctx);
 }
diff --git a/parse-options.h b/parse-options.h
index f2ddef18f7b0..a845a9d95274 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -44,6 +44,7 @@ enum parse_opt_option_flags {
 	PARSE_OPT_LASTARG_DEFAULT = 1 << 4,
 	PARSE_OPT_NODASH = 1 << 5,
 	PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
+	PARSE_OPT_FROM_ALIAS = 1 << 7,
 	PARSE_OPT_SHELL_EVAL = 1 << 8,
 	PARSE_OPT_NOCOMPLETE = 1 << 9,
 	PARSE_OPT_COMP_ARG = 1 << 10,
-- 
gitgitgadget


  parent reply	other threads:[~2021-03-21 16:59 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-08 18:36 [PATCH 0/7] Fix all leaks in t0001 Andrzej Hunt via GitGitGadget
2021-03-08 18:36 ` [PATCH 1/7] symbolic-ref: don't leak shortened refname in check_symref() Andrzej Hunt via GitGitGadget
2021-03-08 19:01   ` Jeff King
2021-03-14 18:07     ` Andrzej Hunt
2021-03-08 18:36 ` [PATCH 2/7] reset: free instead of leaking unneeded ref Andrzej Hunt via GitGitGadget
2021-03-08 19:03   ` Jeff King
2021-03-08 18:36 ` [PATCH 3/7] clone: free or UNLEAK further pointers when finished Andrzej Hunt via GitGitGadget
2021-03-08 19:12   ` Jeff King
2021-03-14 16:56     ` Andrzej Hunt
2021-03-08 18:36 ` [PATCH 4/7] worktree: fix leak in dwim_branch() Andrzej Hunt via GitGitGadget
2021-03-08 19:16   ` Jeff King
2021-03-14 17:56     ` Andrzej Hunt
2021-03-08 18:36 ` [PATCH 5/7] init: remove git_init_db_config() while fixing leaks Andrzej Hunt via GitGitGadget
2021-03-08 19:29   ` Jeff King
2021-03-08 18:36 ` [PATCH 6/7] init-db: silence template_dir leak when converting to absolute path Andrzej Hunt via GitGitGadget
2021-03-08 19:30   ` Jeff King
2021-03-08 18:36 ` [PATCH 7/7] parse-options: don't leak alias help messages Andrzej Hunt via GitGitGadget
2021-03-08 19:46   ` Jeff King
2021-03-14 17:03     ` Andrzej Hunt
2021-03-08 18:55 ` [PATCH 0/7] Fix all leaks in t0001 Jeff King
2021-03-12 23:42   ` Junio C Hamano
2021-03-14 16:54   ` Andrzej Hunt
2021-03-15 16:23     ` Andrzej Hunt
2021-03-08 18:57 ` Junio C Hamano
2021-03-14 16:55   ` Andrzej Hunt
2021-03-14 18:47 ` [PATCH v2 0/9] " Andrzej Hunt via GitGitGadget
2021-03-14 18:47   ` [PATCH v2 1/9] symbolic-ref: don't leak shortened refname in check_symref() Andrzej Hunt via GitGitGadget
2021-03-14 18:47   ` [PATCH v2 2/9] reset: free instead of leaking unneeded ref Andrzej Hunt via GitGitGadget
2021-03-14 18:47   ` [PATCH v2 3/9] clone: free or UNLEAK further pointers when finished Andrzej Hunt via GitGitGadget
2021-03-14 18:47   ` [PATCH v2 4/9] worktree: fix leak in dwim_branch() Andrzej Hunt via GitGitGadget
2021-03-14 18:47   ` [PATCH v2 5/9] init: remove git_init_db_config() while fixing leaks Andrzej Hunt via GitGitGadget
2021-03-14 18:47   ` [PATCH v2 6/9] init-db: silence template_dir leak when converting to absolute path Andrzej Hunt via GitGitGadget
2021-03-14 18:47   ` [PATCH v2 7/9] parse-options: convert bitfield values to use binary shift Andrzej Hunt via GitGitGadget
2021-03-14 20:25     ` Martin Ågren
2021-03-14 22:57       ` Junio C Hamano
2021-03-15 16:20       ` Andrzej Hunt
2021-03-14 18:47   ` [PATCH v2 8/9] parse-options: don't leak alias help messages Andrzej Hunt via GitGitGadget
2021-03-14 19:48     ` Eric Sunshine
2021-03-15 16:20       ` Andrzej Hunt
2021-03-14 20:00     ` Andrzej Hunt
2021-03-14 18:47   ` [PATCH v2 9/9] transport: also free remote_refs in transport_disconnect() Andrzej Hunt via GitGitGadget
2021-03-21 16:58   ` [PATCH v3 0/9] Fix all leaks in t0001 Andrzej Hunt via GitGitGadget
2021-03-21 16:58     ` [PATCH v3 1/9] symbolic-ref: don't leak shortened refname in check_symref() Andrzej Hunt via GitGitGadget
2021-03-21 16:58     ` [PATCH v3 2/9] reset: free instead of leaking unneeded ref Andrzej Hunt via GitGitGadget
2021-03-21 16:58     ` [PATCH v3 3/9] clone: free or UNLEAK further pointers when finished Andrzej Hunt via GitGitGadget
2021-03-21 16:58     ` [PATCH v3 4/9] worktree: fix leak in dwim_branch() Andrzej Hunt via GitGitGadget
2021-03-21 16:58     ` [PATCH v3 5/9] init: remove git_init_db_config() while fixing leaks Andrzej Hunt via GitGitGadget
2021-03-21 16:58     ` [PATCH v3 6/9] init-db: silence template_dir leak when converting to absolute path Andrzej Hunt via GitGitGadget
2021-03-21 16:58     ` [PATCH v3 7/9] parse-options: convert bitfield values to use binary shift Andrzej Hunt via GitGitGadget
2021-03-21 16:58     ` Andrzej Hunt via GitGitGadget [this message]
2021-03-21 16:58     ` [PATCH v3 9/9] transport: also free remote_refs in transport_disconnect() Andrzej Hunt via GitGitGadget
2021-03-21 21:40     ` [PATCH v3 0/9] Fix all leaks in t0001 Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6e46cd332023a579ab0b4c682111c085a634dafb.1616345918.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=ajrhunt@google.com \
    --cc=andrzej@ahunt.org \
    --cc=git@vger.kernel.org \
    --cc=martin.agren@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).