git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 03/11] config: prefer git_config_string_dup() for temp variables

From
Jeff King <peff@peff.net>
Date
Apr 7, 2024, 01:00 UTC
Message-ID
<20240407010037.GC868358@coredump.intra.peff.net>
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
Previous: Jeff KingNext: Eric Sunshine
Message 6 of 23 in “config: do not leak excludes_file”
  1. config: do not leak excludes_fileJunio C Hamano, Apr 6, 2024
  2. [WIP] git_config_pathname() leakfixJunio C Hamano, Apr 6, 2024
  3. 0/12 git_config_string() considered harmfulJeff King, Apr 7, 2024
  4. 01/11 config: make sequencer.c's git_config_string_dup() publicJeff King, Apr 7, 2024
  5. 02/11 config: add git_config_pathname_dup()Jeff King, Apr 7, 2024
  6. 03/11 config: prefer git_config_string_dup() for temp variablesJeff King, Apr 7, 2024
  7. Eric SunshineApr 7, 2024
  8. Jeff KingApr 7, 2024
  9. 04/11 config: use git_config_string_dup() for open-coded equivalentsJeff King, Apr 7, 2024
  10. 05/11 config: use git_config_string_dup() to fix leaky open codingJeff King, Apr 7, 2024
  11. 06/11 config: use git_config_string() in easy casesJeff King, Apr 7, 2024
  12. Eric SunshineApr 7, 2024
  13. Jeff KingApr 7, 2024
  14. 07/11 config: use git_config_pathname_dup() in easy casesJeff King, Apr 7, 2024
  15. 08/11 http: use git_config_string_dup()Jeff King, Apr 7, 2024
  16. 09/11 merge: use git_config_string_dup() for pull strategiesJeff King, Apr 7, 2024
  17. 10/11 userdiff: use git_config_string_dup() when we canJeff King, Apr 7, 2024
  18. 11/11 blame: use "dup" string_list for ignore-revs filesJeff King, Apr 7, 2024
  19. Eric SunshineApr 7, 2024
  20. Jeff KingApr 7, 2024
  21. Rubén JustoApr 7, 2024
  22. Jeff KingApr 8, 2024
  23. Rubén JustoApr 11, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.