git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "Rubén Justo" <rjusto@gmail.com>
Subject: [PATCH 03/11] config: prefer git_config_string_dup() for temp variables
Date: Sat, 6 Apr 2024 21:00:37 -0400	[thread overview]
Message-ID: <20240407010037.GC868358@coredump.intra.peff.net> (raw)
In-Reply-To: <20240407005656.GA436890@coredump.intra.peff.net>

In some cases we may use git_config_string() or git_config_pathname() to
read a value into a temporary variable, and then either hand off memory
ownership of the new variable or free it. These are not potential leaks,
since we know that there is no previous value we are overwriting.

However, it's worth converting these to use git_config_string_dup() and
git_config_pathname_dup(). It makes it easier to audit for leaky cases,
and possibly we can get rid of the leak-prone functions in the future.
Plus it lets the const-ness of our variables match their expected memory
ownership, which avoids some casts when calling free().

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/blame.c        |  4 ++--
 builtin/config.c       |  6 +++---
 builtin/receive-pack.c |  6 +++---
 fetch-pack.c           |  6 +++---
 fsck.c                 |  6 +++---
 remote.c               | 28 ++++++++++++++--------------
 setup.c                |  6 +++---
 7 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index db1f56de61..0b07ceb110 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -718,10 +718,10 @@ static int git_blame_config(const char *var, const char *value,
 		return 0;
 	}
 	if (!strcmp(var, "blame.ignorerevsfile")) {
-		const char *str;
+		char *str = NULL;
 		int ret;
 
-		ret = git_config_pathname(&str, var, value);
+		ret = git_config_pathname_dup(&str, var, value);
 		if (ret)
 			return ret;
 		string_list_insert(&ignore_revs_file_list, str);
diff --git a/builtin/config.c b/builtin/config.c
index 0015620dde..fc5d96bb4c 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -282,11 +282,11 @@ static int format_config(struct strbuf *buf, const char *key_,
 			else
 				strbuf_addstr(buf, v ? "true" : "false");
 		} else if (type == TYPE_PATH) {
-			const char *v;
-			if (git_config_pathname(&v, key_, value_) < 0)
+			char *v = NULL;
+			if (git_config_pathname_dup(&v, key_, value_) < 0)
 				return -1;
 			strbuf_addstr(buf, v);
-			free((char *)v);
+			free(v);
 		} else if (type == TYPE_EXPIRY_DATE) {
 			timestamp_t t;
 			if (git_config_expiry_date(&t, key_, value_) < 0)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 56d8a77ed7..15ed81a3f6 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -168,13 +168,13 @@ static int receive_pack_config(const char *var, const char *value,
 	}
 
 	if (strcmp(var, "receive.fsck.skiplist") == 0) {
-		const char *path;
+		char *path = NULL;
 
-		if (git_config_pathname(&path, var, value))
+		if (git_config_pathname_dup(&path, var, value))
 			return 1;
 		strbuf_addf(&fsck_msg_types, "%cskiplist=%s",
 			fsck_msg_types.len ? ',' : '=', path);
-		free((char *)path);
+		free(path);
 		return 0;
 	}
 
diff --git a/fetch-pack.c b/fetch-pack.c
index 091f9a80a9..fd59c497b4 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1863,13 +1863,13 @@ static int fetch_pack_config_cb(const char *var, const char *value,
 	const char *msg_id;
 
 	if (strcmp(var, "fetch.fsck.skiplist") == 0) {
-		const char *path;
+		char *path = NULL;
 
-		if (git_config_pathname(&path, var, value))
+		if (git_config_pathname_dup(&path, var, value))
 			return 1;
 		strbuf_addf(&fsck_msg_types, "%cskiplist=%s",
 			fsck_msg_types.len ? ',' : '=', path);
-		free((char *)path);
+		free(path);
 		return 0;
 	}
 
diff --git a/fsck.c b/fsck.c
index 78af29d264..a9d27a660f 100644
--- a/fsck.c
+++ b/fsck.c
@@ -1274,13 +1274,13 @@ int git_fsck_config(const char *var, const char *value,
 	const char *msg_id;
 
 	if (strcmp(var, "fsck.skiplist") == 0) {
-		const char *path;
+		char *path = NULL;
 		struct strbuf sb = STRBUF_INIT;
 
-		if (git_config_pathname(&path, var, value))
+		if (git_config_pathname_dup(&path, var, value))
 			return 1;
 		strbuf_addf(&sb, "skiplist=%s", path);
-		free((char *)path);
+		free(path);
 		fsck_set_msg_types(options, sb.buf);
 		strbuf_release(&sb);
 		return 0;
diff --git a/remote.c b/remote.c
index 2b650b813b..09912bebf1 100644
--- a/remote.c
+++ b/remote.c
@@ -428,38 +428,38 @@ static int handle_config(const char *key, const char *value,
 	else if (!strcmp(subkey, "prunetags"))
 		remote->prune_tags = git_config_bool(key, value);
 	else if (!strcmp(subkey, "url")) {
-		const char *v;
-		if (git_config_string(&v, key, value))
+		char *v = NULL;
+		if (git_config_string_dup(&v, key, value))
 			return -1;
 		add_url(remote, v);
 	} else if (!strcmp(subkey, "pushurl")) {
-		const char *v;
-		if (git_config_string(&v, key, value))
+		char *v = NULL;
+		if (git_config_string_dup(&v, key, value))
 			return -1;
 		add_pushurl(remote, v);
 	} else if (!strcmp(subkey, "push")) {
-		const char *v;
-		if (git_config_string(&v, key, value))
+		char *v = NULL;
+		if (git_config_string_dup(&v, key, value))
 			return -1;
 		refspec_append(&remote->push, v);
-		free((char *)v);
+		free(v);
 	} else if (!strcmp(subkey, "fetch")) {
-		const char *v;
-		if (git_config_string(&v, key, value))
+		char *v = NULL;
+		if (git_config_string_dup(&v, key, value))
 			return -1;
 		refspec_append(&remote->fetch, v);
-		free((char *)v);
+		free(v);
 	} else if (!strcmp(subkey, "receivepack")) {
-		const char *v;
-		if (git_config_string(&v, key, value))
+		char *v = NULL;
+		if (git_config_string_dup(&v, key, value))
 			return -1;
 		if (!remote->receivepack)
 			remote->receivepack = v;
 		else
 			error(_("more than one receivepack given, using the first"));
 	} else if (!strcmp(subkey, "uploadpack")) {
-		const char *v;
-		if (git_config_string(&v, key, value))
+		char *v = NULL;
+		if (git_config_string_dup(&v, key, value))
 			return -1;
 		if (!remote->uploadpack)
 			remote->uploadpack = v;
diff --git a/setup.c b/setup.c
index f4b32f76e3..9f35a27978 100644
--- a/setup.c
+++ b/setup.c
@@ -1176,13 +1176,13 @@ static int safe_directory_cb(const char *key, const char *value,
 	} else if (!strcmp(value, "*")) {
 		data->is_safe = 1;
 	} else {
-		const char *interpolated = NULL;
+		char *interpolated = NULL;
 
-		if (!git_config_pathname(&interpolated, key, value) &&
+		if (!git_config_pathname_dup(&interpolated, key, value) &&
 		    !fspathcmp(data->path, interpolated ? interpolated : value))
 			data->is_safe = 1;
 
-		free((char *)interpolated);
+		free(interpolated);
 	}
 
 	return 0;
-- 
2.44.0.872.g288abe5b5b



  parent reply	other threads:[~2024-04-07  1:00 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-06 18:11 [PATCH] config: do not leak excludes_file Junio C Hamano
2024-04-06 19:17 ` [WIP] git_config_pathname() leakfix Junio C Hamano
2024-04-07  0:56 ` [PATCH 0/12] git_config_string() considered harmful Jeff King
2024-04-07  0:58   ` [PATCH 01/11] config: make sequencer.c's git_config_string_dup() public Jeff King
2024-04-07  1:00   ` [PATCH 02/11] config: add git_config_pathname_dup() Jeff King
2024-04-07  1:00   ` Jeff King [this message]
2024-04-07  1:50     ` [PATCH 03/11] config: prefer git_config_string_dup() for temp variables Eric Sunshine
2024-04-07  2:45       ` Jeff King
2024-04-07  1:01   ` [PATCH 04/11] config: use git_config_string_dup() for open-coded equivalents Jeff King
2024-04-07  1:01   ` [PATCH 05/11] config: use git_config_string_dup() to fix leaky open coding Jeff King
2024-04-07  1:02   ` [PATCH 06/11] config: use git_config_string() in easy cases Jeff King
2024-04-07  2:05     ` Eric Sunshine
2024-04-07  2:47       ` Jeff King
2024-04-07  1:03   ` [PATCH 07/11] config: use git_config_pathname_dup() " Jeff King
2024-04-07  1:03   ` [PATCH 08/11] http: use git_config_string_dup() Jeff King
2024-04-07  1:04   ` [PATCH 09/11] merge: use git_config_string_dup() for pull strategies Jeff King
2024-04-07  1:04   ` [PATCH 10/11] userdiff: use git_config_string_dup() when we can Jeff King
2024-04-07  1:07   ` [PATCH 11/11] blame: use "dup" string_list for ignore-revs files Jeff King
2024-04-07  2:42     ` Eric Sunshine
2024-04-07  1:08   ` [PATCH 0/12] git_config_string() considered harmful Jeff King
2024-04-07 17:58   ` Rubén Justo
2024-04-08 20:55     ` Jeff King
2024-04-11 23:36       ` Rubén Justo

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20240407010037.GC868358@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=rjusto@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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).