git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Andrei Rybak" <rybak.a.v@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v3 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()
Date: Wed, 30 Jun 2021 16:06:14 +0200	[thread overview]
Message-ID: <patch-1.3-3d0d7a8e8b5-20210630T140339Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-0.3-00000000000-20210630T140339Z-avarab@gmail.com>

Fix a memory leak from the prefix_filename() function introduced with
its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
2017-03-20).

As noted in that commit the leak was intentional as a part of being
sloppy about freeing resources just before we exit, I'm changing this
because I'll be fixing other memory leaks in the bundle API (including
the library version) in subsequent commits. It's easier to reason
about those fixes if valgrind runs cleanly at the end without any
leaks whatsoever.

An earlier version of this change went out of its way to not leak
memory on the die() codepaths here, but that was deemed too verbose to
worry about in a built-in that's dying anyway. The only reason we'd
need that is to appease a mode like SANITIZE=leak within the scope of
an entire test file.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bundle.c | 62 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 21 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index ea6948110b0..15e2bd61965 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc,
 		const char* prefix,
 		const char * const usagestr[],
 		const struct option options[],
-		const char **bundle_file) {
+		char **bundle_file) {
 	int newargc;
 	newargc = parse_options(argc, argv, NULL, options, usagestr,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
@@ -61,7 +61,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 	int progress = isatty(STDERR_FILENO);
 	struct strvec pack_opts;
 	int version = -1;
-
+	int ret;
 	struct option options[] = {
 		OPT_SET_INT('q', "quiet", &progress,
 			    N_("do not show progress meter"), 0),
@@ -76,7 +76,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 			    N_("specify bundle format version")),
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_create_usage, options, &bundle_file);
@@ -94,75 +94,95 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 
 	if (!startup_info->have_repository)
 		die(_("Need a repository to create a bundle."));
-	return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
+	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
+	free(bundle_file);
+	return ret;
 }
 
 static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
 	int quiet = 0;
-
+	int ret;
 	struct option options[] = {
 		OPT_BOOL('q', "quiet", &quiet,
 			    N_("do not show bundle details")),
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 	close(bundle_fd);
-	if (verify_bundle(the_repository, &header, !quiet))
-		return 1;
+	if (verify_bundle(the_repository, &header, !quiet)) {
+		ret = 1;
+		goto cleanup;
+	}
+
 	fprintf(stderr, _("%s is okay\n"), bundle_file);
-	return 0;
+	ret = 0;
+cleanup:
+	free(bundle_file);
+	return ret;
 }
 
 static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
-
+	int ret;
 	struct option options[] = {
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_list_heads_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 	close(bundle_fd);
-	return !!list_bundle_refs(&header, argc, argv);
+	ret = !!list_bundle_refs(&header, argc, argv);
+cleanup:
+	free(bundle_file);
+	return ret;
 }
 
 static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
-
+	int ret;
 	struct option options[] = {
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_unbundle_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 	if (!startup_info->have_repository)
 		die(_("Need a repository to unbundle."));
-	return !!unbundle(the_repository, &header, bundle_fd, 0) ||
+	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
 		list_bundle_refs(&header, argc, argv);
+cleanup:
+	free(bundle_file);
+	return ret;
 }
 
 int cmd_bundle(int argc, const char **argv, const char *prefix)
-- 
2.32.0.613.g8e17abc2eb


  reply	other threads:[~2021-06-30 14:06 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-17 11:21 [PATCH 0/3] bundle.c: remove "ref_list" in favor of string-list.c API Ævar Arnfjörð Bjarmason
2021-06-17 11:21 ` [PATCH 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle() Ævar Arnfjörð Bjarmason
2021-06-17 11:21 ` [PATCH 2/3] bundle.c: use a temporary variable for OIDs and names Ævar Arnfjörð Bjarmason
2021-06-17 11:21 ` [PATCH 3/3] bundle: remove "ref_list" in favor of string-list.c API Ævar Arnfjörð Bjarmason
2021-06-19  2:12   ` Andrei Rybak
2021-06-21 15:16 ` [PATCH v2 0/3] bundle.c: " Ævar Arnfjörð Bjarmason
2021-06-21 15:16   ` [PATCH v2 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle() Ævar Arnfjörð Bjarmason
2021-06-24 16:54     ` Jeff King
2021-06-24 19:28       ` Ævar Arnfjörð Bjarmason
2021-06-21 15:16   ` [PATCH v2 2/3] bundle.c: use a temporary variable for OIDs and names Ævar Arnfjörð Bjarmason
2021-06-21 15:16   ` [PATCH v2 3/3] bundle: remove "ref_list" in favor of string-list.c API Ævar Arnfjörð Bjarmason
2021-06-24 17:11     ` Jeff King
2021-06-24 19:31       ` Ævar Arnfjörð Bjarmason
2021-06-29  1:02       ` Junio C Hamano
2021-06-24 17:14   ` [PATCH v2 0/3] bundle.c: " Jeff King
2021-06-30 14:06   ` [PATCH v3 0/3] Ævar Arnfjörð Bjarmason
2021-06-30 14:06     ` Ævar Arnfjörð Bjarmason [this message]
2021-06-30 17:26       ` [PATCH v3 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle() Jeff King
2021-06-30 18:00         ` Ævar Arnfjörð Bjarmason
2021-07-01 15:41           ` Jeff King
2021-06-30 14:06     ` [PATCH v3 2/3] bundle.c: use a temporary variable for OIDs and names Ævar Arnfjörð Bjarmason
2021-06-30 14:06     ` [PATCH v3 3/3] bundle: remove "ref_list" in favor of string-list.c API Ævar Arnfjörð Bjarmason
2021-06-30 21:18       ` Junio C Hamano
2021-06-30 17:34     ` [PATCH v3 0/3] Jeff King
2021-06-30 17:45       ` Jeff King
2021-06-30 18:00         ` Ævar Arnfjörð Bjarmason
2021-07-01 10:53           ` Ævar Arnfjörð Bjarmason
2021-07-02  9:57     ` [PATCH v4 0/3] bundle.c: remove "ref_list" in favor of string-list.c API Ævar Arnfjörð Bjarmason
2021-07-02  9:57       ` [PATCH v4 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle() Ævar Arnfjörð Bjarmason
2021-07-02  9:57       ` [PATCH v4 2/3] bundle.c: use a temporary variable for OIDs and names Ævar Arnfjörð Bjarmason
2021-07-02  9:57       ` [PATCH v4 3/3] bundle: remove "ref_list" in favor of string-list.c API Ævar Arnfjörð Bjarmason
2021-07-03 10:52       ` [PATCH v4 0/3] bundle.c: " Jeff King
2021-07-03 11:28         ` Ævar Arnfjörð Bjarmason

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=patch-1.3-3d0d7a8e8b5-20210630T140339Z-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=rybak.a.v@gmail.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).