threads / patch / 37203

patch, 5 partscoverity mixed bag

Subject: [PATCH 0/5] coverity mixed bag

## tl;dr

8 messages between Jul 24, 2014 and Jul 29, 2014. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Jeff King· Jul 24, 2014, 04:39 UTC · lore

Since Stefan has recently started feeding git builds to coverity, I spent a few minutes poking through the results. There are tons of false positives, so there is some work to be done there with tweaking our coverity models. But there are some real issues, too. Here are fixes for the handful that I looked at.

  [1/5]: receive-pack: don't copy "dir" parameter
  [2/5]: free ref string returned by dwim_ref
  [3/5]: transport: fix leaks in refs_from_alternate_cb
  [4/5]: fix memory leak parsing core.commentchar
  [5/5]: apply: avoid possible bogus pointer
-Peff
Jeff King· Jul 24, 2014, 04:40 UTC · re: Jeff King · lore

[PATCH 1/5] receive-pack: don't copy "dir" parameter

We used to do this so could pass a mutable string to enter_repo. But since 1c64b48 (enter_repo: do not modify input, 2011-10-04), this is not necessary.

The resulting code is simpler, and it fixes a minor leak.
Signed-off-by: Jeff King <peff@peff.net>
---
If you are wondering whether upload-pack needs the same treatment, we
already did it in 6379dd0.
 builtin/receive-pack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin/receive-pack.c +2 −2
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 92561bf..f93ac45 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1122,7 +1122,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 	int advertise_refs = 0;
 	int stateless_rpc = 0;
 	int i;
-	char *dir = NULL;
+	const char *dir = NULL;
 	struct command *commands;
 	struct sha1_array shallow = SHA1_ARRAY_INIT;
 	struct sha1_array ref = SHA1_ARRAY_INIT;
@@ -1157,7 +1157,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		}
 		if (dir)
 			usage(receive_pack_usage);
-		dir = xstrdup(arg);
+		dir = arg;
 	}
 	if (!dir)
 		usage(receive_pack_usage);
-- 
2.0.0.566.gfe3e6b2
Jeff King· Jul 24, 2014, 04:41 UTC · re: Jeff King · lore

[PATCH 2/5] free ref string returned by dwim_ref

A call to "dwim_ref(name, len, flags, &ref)" will allocate a new string in "ref" to return the exact ref we found. We do not consistently free it in all code paths, leading to small leaks. The worst is in get_sha1_basic, which may be called many times (e.g., by "cat-file --batch"), though it is relatively unlikely, as it only triggers on a bogus reflog specification.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/rev-parse.c   | 1 +
 builtin/show-branch.c | 1 +
 sha1_name.c           | 4 +++-
 3 files changed, 5 insertions(+), 1 deletion(-)
Show changes to 3 files +5 −1

builtin/rev-parse.c, builtin/show-branch.c, sha1_name.c

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 8102aaa..d85e08c 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -151,6 +151,7 @@ static void show_rev(int type, const unsigned char *sha1, const char *name)
 				error("refname '%s' is ambiguous", name);
 				break;
 			}
+			free(full);
 		} else {
 			show_with_type(type, name);
 		}
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 5fd4e4e..298c95e 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -777,6 +777,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			sprintf(nth_desc, "%s@{%d}", *av, base+i);
 			append_ref(nth_desc, sha1, 1);
 		}
+		free(ref);
 	}
 	else if (all_heads + all_remotes)
 		snarf_refs(all_heads, all_remotes);
diff --git a/sha1_name.c b/sha1_name.c
index 6ccd3a5..63ee66f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -540,8 +540,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 			char *tmp = xstrndup(str + at + 2, reflog_len);
 			at_time = approxidate_careful(tmp, &errors);
 			free(tmp);
-			if (errors)
+			if (errors) {
+				free(real_ref);
 				return -1;
+			}
 		}
 		if (read_ref_at(real_ref, at_time, nth, sha1, NULL,
 				&co_time, &co_tz, &co_cnt)) {
-- 
2.0.0.566.gfe3e6b2
Jeff King· Jul 24, 2014, 04:41 UTC · re: Jeff King · lore

[PATCH 3/5] transport: fix leaks in refs_from_alternate_cb

The function starts by creating a copy of the static buffer returned by real_path, but forgets to free it in the error code paths. We can solve this by jumping to the cleanup code that is already there.

Signed-off-by: Jeff King <peff@peff.net>
---
 transport.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to transport.c +3 −2
diff --git a/transport.c b/transport.c
index 3e42570..297e981 100644
--- a/transport.c
+++ b/transport.c
@@ -1359,11 +1359,11 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,
 	while (other[len-1] == '/')
 		other[--len] = '\0';
 	if (len < 8 || memcmp(other + len - 8, "/objects", 8))
-		return 0;
+		goto out;
 	/* Is this a git repository with refs? */
 	memcpy(other + len - 8, "/refs", 6);
 	if (!is_directory(other))
-		return 0;
+		goto out;
 	other[len - 8] = '\0';
 	remote = remote_get(other);
 	transport = transport_get(remote, other);
@@ -1372,6 +1372,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,
 	     extra = extra->next)
 		cb->fn(extra, cb->data);
 	transport_disconnect(transport);
+out:
 	free(other);
 	return 0;
 }
-- 
2.0.0.566.gfe3e6b2
Jeff King· Jul 24, 2014, 04:42 UTC · re: Jeff King · lore

[PATCH 4/5] fix memory leak parsing core.commentchar

When we see the core.commentchar config option, we extract the string with git_config_string, which does two things:

  1. It complains via config_error_nonbool if there is no
     string value.
  2. It makes a copy of the string.

Since we immediately parse the string into its single-character value, we only care about (1). And in fact (2) is a detriment, as it means we leak the copy. Instead, let's just check the pointer value ourselves, and parse directly from the const string we already have.

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
Show changes to config.c +5 −7
diff --git a/config.c b/config.c
index 9767c4b..058505c 100644
--- a/config.c
+++ b/config.c
@@ -817,14 +817,12 @@ static int git_default_core_config(const char *var, const char *value)
 		return git_config_string(&editor_program, var, value);
 
 	if (!strcmp(var, "core.commentchar")) {
-		const char *comment;
-		int ret = git_config_string(&comment, var, value);
-		if (ret)
-			return ret;
-		else if (!strcasecmp(comment, "auto"))
+		if (!value)
+			return config_error_nonbool(var);
+		else if (!strcasecmp(value, "auto"))
 			auto_comment_line_char = 1;
-		else if (comment[0] && !comment[1]) {
-			comment_line_char = comment[0];
+		else if (value[0] && !value[1]) {
+			comment_line_char = value[0];
 			auto_comment_line_char = 0;
 		} else
 			return error("core.commentChar should only be one character");
-- 
2.0.0.566.gfe3e6b2
Jeff King· Jul 24, 2014, 04:43 UTC · re: Jeff King · lore

[PATCH 5/5] apply: avoid possible bogus pointer

When parsing "index" lines from a git-diff, we look for a space followed by the mode. If we don't have a space, then we set our pointer to the end-of-line. However, we don't double-check that our end-of-line pointer is valid (e.g., if we got a truncated diff input), which could lead to some wrap-around pointer arithmetic.

In most cases this would probably get caught by our "40 < len" check later in the function, but to be on the safe side, let's just use strchrnul to treat end-of-string the same as end-of-line.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/apply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/apply.c +1 −1
diff --git a/builtin/apply.c b/builtin/apply.c
index 9f8f5ba..be2b4ce 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1075,7 +1075,7 @@ static int gitdiff_index(const char *line, struct patch *patch)
 
 	line = ptr + 2;
 	ptr = strchr(line, ' ');
-	eol = strchr(line, '\n');
+	eol = strchrnul(line, '\n');
 
 	if (!ptr || eol < ptr)
 		ptr = eol;
-- 
2.0.0.566.gfe3e6b2
Junio C Hamano· Jul 24, 2014, 20:33 UTC · re: Jeff King · lore

Re: [PATCH 0/5] coverity mixed bag

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> Since Stefan has recently started feeding git builds to coverity, I
> spent a few minutes poking through the results. There are tons of false
> positives, so there is some work to be done there with tweaking our
> coverity models. But there are some real issues, too. Here are fixes for
> the handful that I looked at.
>
>   [1/5]: receive-pack: don't copy "dir" parameter
>   [2/5]: free ref string returned by dwim_ref
>   [3/5]: transport: fix leaks in refs_from_alternate_cb
>   [4/5]: fix memory leak parsing core.commentchar
>   [5/5]: apply: avoid possible bogus pointer
>
> -Peff

Thanks, they all make sense to me. I'd probably have to wiggle 4/5 a bit to port the fix over to both maint and master, though.

Stefan Beller· Jul 29, 2014, 05:36 UTC · re: Jeff King · lore

Re: [PATCH 0/5] coverity mixed bag

On 24.07.2014 06:39, Jeff King wrote:
Show 14 quoted lines
> Since Stefan has recently started feeding git builds to coverity, I
> spent a few minutes poking through the results. There are tons of false
> positives, so there is some work to be done there with tweaking our
> coverity models. But there are some real issues, too. Here are fixes for
> the handful that I looked at.
> 
>   [1/5]: receive-pack: don't copy "dir" parameter
>   [2/5]: free ref string returned by dwim_ref
>   [3/5]: transport: fix leaks in refs_from_alternate_cb
>   [4/5]: fix memory leak parsing core.commentchar
>   [5/5]: apply: avoid possible bogus pointer
> 
> -Peff
> 

As this patchset is in, we have coverity scan now finding 10 defects fixed.

Thanks, Stefan

← back to recent threads