[PATCH 0/5] coverity mixed bag

[PATCH 0/5] coverity mixed bag

[Jeff King]

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

Re: [PATCH 0/5] coverity mixed bag

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Re: [PATCH 0/5] coverity mixed bag

[Stefan Beller]

On 24.07.2014 06:39, Jeff King wrote:
> 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