[PATCH 0/7] cleanups: intro

[PATCH 0/7] cleanups: intro

[Serge E. Hallyn]

While debating whether to run git-daemon at home, I figured I'd
run some static analysis on the source.  The following patches
are all pretty trivial, addressing things the tool pointed out.

thanks,
-serge

[PATCH 2/7] cleanups: Fix potential bugs in connect.c

[Serge E. Hallyn]

The strncmp for ACK was ACK does not include the final space.
Presumably either we should either remove the trailing space,
or compare 4 chars (as this patch does).

'path' is sometimes strdup'ed, but never freed.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 connect.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

1b4f45918d8c1f7d579840c7bb1fe3fe1967b6c5
diff --git a/connect.c b/connect.c
index 3f2d65c..6a8f8a6 100644
--- a/connect.c
+++ b/connect.c
@@ -74,7 +74,7 @@ int get_ack(int fd, unsigned char *resul
 		line[--len] = 0;
 	if (!strcmp(line, "NAK"))
 		return 0;
-	if (!strncmp(line, "ACK ", 3)) {
+	if (!strncmp(line, "ACK ", 4)) {
 		if (!get_sha1_hex(line+4, result_sha1)) {
 			if (strstr(line+45, "continue"))
 				return 2;
@@ -567,6 +567,7 @@ int git_connect(int fd[2], char *url, co
 	int pipefd[2][2];
 	pid_t pid;
 	enum protocol protocol = PROTO_LOCAL;
+	int free_path = 0;
 
 	host = strstr(url, "://");
 	if(host) {
@@ -610,16 +611,23 @@ int git_connect(int fd[2], char *url, co
 		char *ptr = path;
 		if (path[1] == '~')
 			path++;
-		else
+		else {
 			path = strdup(ptr);
+			free_path = 1;
+		}
 
 		*ptr = '\0';
 	}
 
 	if (protocol == PROTO_GIT) {
+		int ret;
 		if (git_use_proxy(host))
-			return git_proxy_connect(fd, prog, host, path);
-		return git_tcp_connect(fd, prog, host, path);
+			ret = git_proxy_connect(fd, prog, host, path);
+		else
+			ret = git_tcp_connect(fd, prog, host, path);
+		if (free_path)
+			free(path);
+		return ret;
 	}
 
 	if (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)
@@ -659,6 +667,8 @@ int git_connect(int fd[2], char *url, co
 	fd[1] = pipefd[1][1];
 	close(pipefd[0][1]);
 	close(pipefd[1][0]);
+	if (free_path)
+		free(path);
 	return pid;
 }
 
-- 
1.2.5

[PATCH 3/7] cleanups: Remove unused vars from combine-diff.c

[Serge E. Hallyn]

Mod_type in particular sure looks like it wants to be used, but isn't.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 combine-diff.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

5a7c4023402dd2420a4596cdd92a1a46fd3e5c14
diff --git a/combine-diff.c b/combine-diff.c
index 9bd27f8..9445e86 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -589,7 +589,7 @@ static int show_patch_diff(struct combin
 			   struct diff_options *opt)
 {
 	unsigned long result_size, cnt, lno;
-	char *result, *cp, *ep;
+	char *result, *cp;
 	struct sline *sline; /* survived lines */
 	int mode_differs = 0;
 	int i, show_hunks, shown_header = 0;
@@ -641,7 +641,6 @@ static int show_patch_diff(struct combin
 		cnt++; /* incomplete line */
 
 	sline = xcalloc(cnt+2, sizeof(*sline));
-	ep = result;
 	sline[0].bol = result;
 	for (lno = 0; lno <= cnt + 1; lno++) {
 		sline[lno].lost_tail = &sline[lno].lost_head;
@@ -752,7 +751,7 @@ static int show_patch_diff(struct combin
 
 static void show_raw_diff(struct combine_diff_path *p, int num_parent, const char *header, struct diff_options *opt)
 {
-	int i, offset, mod_type = 'A';
+	int i, offset;
 	const char *prefix;
 	int line_termination, inter_name_termination;
 
@@ -764,13 +763,6 @@ static void show_raw_diff(struct combine
 	if (header)
 		printf("%s%c", header, line_termination);
 
-	for (i = 0; i < num_parent; i++) {
-		if (p->parent[i].mode)
-			mod_type = 'M';
-	}
-	if (!p->mode)
-		mod_type = 'D';
-
 	if (opt->output_format == DIFF_FORMAT_RAW) {
 		offset = strlen(COLONS) - num_parent;
 		if (offset < 0)
-- 
1.2.5

[PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c

[Serge E. Hallyn]

Address two reports from an automatic code analyzer:

1. In logreport, it is possible to write \0 one
character past the end of buf[].

2. In socksetup, socklist can be leaked when returning
if set_reuse_addr().  Note: dunno why this case returns...

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 daemon.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

5b3e0254d34da582b7593084356c88a923f42a49
diff --git a/daemon.c b/daemon.c
index a1ccda3..7ac1bc7 100644
--- a/daemon.c
+++ b/daemon.c
@@ -65,8 +65,8 @@ static void logreport(int priority, cons
 	 * we have space for our own LF and NUL after the "meat" of the
 	 * message, so truncate it at maxlen - 1.
 	 */
-	if (msglen > maxlen - 1)
-		msglen = maxlen - 1;
+	if (msglen > maxlen - 2)
+		msglen = maxlen - 2;
 	else if (msglen < 0)
 		msglen = 0; /* Protect against weird return values. */
 	buflen += msglen;
@@ -535,6 +535,7 @@ static int socksetup(int port, int **soc
 
 		if (set_reuse_addr(sockfd)) {
 			close(sockfd);
+			free(socklist);
 			return 0;	/* not fatal */
 		}
 
-- 
1.2.5

Re: [PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c

[Junio C Hamano]

"Serge E. Hallyn" <serue@us.ibm.com> writes:

> Address two reports from an automatic code analyzer:
>
> 1. In logreport, it is possible to write \0 one
> character past the end of buf[].

I am perhaps slower than I usually am today, but it seems to me
that the code caps msglen to (maxlen-1) and then adds that to
buflen.

Now, maxlen is (sizeof(buf)-buflen-1), so that means after
the "buflen += msglen" happens, buflen is at most:

	buflen + (sizeof(buf)-buflen-1) - 1
        = sizeof(buf) - 2

And then "buf[buflen++] = '\n'; buf[buflen] = '\0'" happens.
'\n' is written at sizeof(buf)-2 (or lower index than that) and
'\0' is written at sizeof(buf)-1 (or lower).  I am unsure how it
steps beyond the end...

> 2. In socksetup, socklist can be leaked when returning
> if set_reuse_addr().  Note: dunno why this case returns...

I am not sure why this part returns either.  It appears to me
that it should just keep going just like the cases where
bind/listen fails.

Re: [PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c

[Serge E. Hallyn]

Quoting Junio C Hamano (junkio@cox.net):
> "Serge E. Hallyn" <serue@us.ibm.com> writes:
> 
> > Address two reports from an automatic code analyzer:
> >
> > 1. In logreport, it is possible to write \0 one
> > character past the end of buf[].
> 
> I am perhaps slower than I usually am today, but it seems to me
> that the code caps msglen to (maxlen-1) and then adds that to
> buflen.
> 
> Now, maxlen is (sizeof(buf)-buflen-1), so that means after
> the "buflen += msglen" happens, buflen is at most:
> 
> 	buflen + (sizeof(buf)-buflen-1) - 1
>         = sizeof(buf) - 2
> 
> And then "buf[buflen++] = '\n'; buf[buflen] = '\0'" happens.
> '\n' is written at sizeof(buf)-2 (or lower index than that) and
> '\0' is written at sizeof(buf)-1 (or lower).  I am unsure how it
> steps beyond the end...

Argh, I had to pull out a sheet of paper, but you are right.  I
misread, and the warning must be about the case where the
snprint "[%ld] " prints out 1023 characters.

> > 2. In socksetup, socklist can be leaked when returning
> > if set_reuse_addr().  Note: dunno why this case returns...
> 
> I am not sure why this part returns either.  It appears to me
> that it should just keep going just like the cases where
> bind/listen fails.

Then perhaps the following is more appropriate.

thanks,
-serge

From: Serge E. Hallyn <serue@us.ibm.com>
Subject: [PATCH] socksetup: don't return on set_reuse_addr() error

The set_reuse_addr() error case was the only error case in
socklist() where we returned rather than continued.  Not sure
why.  Either we must free the socklist, or continue.  This patch
continues on error.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 daemon.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

b589029e3187eed51c3fe6a2715f51bea2159786
diff --git a/daemon.c b/daemon.c
index a1ccda3..776749e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -535,7 +535,7 @@ static int socksetup(int port, int **soc
 
 		if (set_reuse_addr(sockfd)) {
 			close(sockfd);
-			return 0;	/* not fatal */
+			continue;
 		}
 
 		if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
-- 
1.2.5

Re: [PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c

[Junio C Hamano]

"Serge E. Hallyn" <serue@us.ibm.com> writes:

> Argh, I had to pull out a sheet of paper, but you are right.  I
> misread, and the warning must be about the case where the
> snprint "[%ld] " prints out 1023 characters.

If snprintf(buf, sizeof(buf), "[%ld] ", (long) getpid()) is
judged to possibly overrun the buffer by your static analysis
tool, I think the tool is broken.  It at least should know how
big a printed long can be.  It would earn bonus points if it can
warn me when sizeof(buf) is sufficiently small, say 40 bytes,
with a message like "on future architectures with 128-bit long
this code might break" ;-).

[PATCH 6/7] cleanups: prevent leak of two strduped strings in config.c

[Serge E. Hallyn]

Config_filename and lockfile are strduped and then leaked in
git_config_set_multivar.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 config.c |   37 +++++++++++++++++++++++++++----------
 1 files changed, 27 insertions(+), 10 deletions(-)

c96b9a1a358f8929a2525dd4e15551d286aec361
diff --git a/config.c b/config.c
index 95ec349..a37bb2d 100644
--- a/config.c
+++ b/config.c
@@ -420,6 +420,7 @@ int git_config_set_multivar(const char* 
 {
 	int i;
 	int fd, in_fd;
+	int ret;
 	char* config_filename = strdup(git_path("config"));
 	char* lock_file = strdup(git_path("config.lock"));
 	const char* last_dot = strrchr(key, '.');
@@ -431,7 +432,8 @@ int git_config_set_multivar(const char* 
 
 	if (last_dot == NULL) {	
 		fprintf(stderr, "key does not contain a section: %s\n", key);
-		return 2;
+		ret = 2;
+		goto out_free;
 	}
 	store.baselen = last_dot - key;
 
@@ -447,7 +449,8 @@ int git_config_set_multivar(const char* 
 				 (i == store.baselen+1 && !isalpha(key[i])))) {
 			fprintf(stderr, "invalid key: %s\n", key);
 			free(store.key);
-			return 1;
+			ret = 1;
+			goto out_free;
 		} else
 			store.key[i] = tolower(key[i]);
 	store.key[i] = 0;
@@ -460,7 +463,8 @@ int git_config_set_multivar(const char* 
 	if (fd < 0) {
 		fprintf(stderr, "could not lock config file\n");
 		free(store.key);
-		return -1;
+		ret = -1;
+		goto out_free;
 	}
 
 	/*
@@ -475,13 +479,15 @@ int git_config_set_multivar(const char* 
 			      strerror(errno));
 			close(fd);
 			unlink(lock_file);
-			return 3; /* same as "invalid config file" */
+			ret = 3; /* same as "invalid config file" */
+			goto out_free;
 		}
 		/* if nothing to unset, error out */
 		if (value == NULL) {
 			close(fd);
 			unlink(lock_file);
-			return 5;
+			ret = 5;
+			goto out_free;
 		}
 
 		store.key = (char*)key;
@@ -507,7 +513,8 @@ int git_config_set_multivar(const char* 
 				fprintf(stderr, "Invalid pattern: %s\n",
 					value_regex);
 				free(store.value_regex);
-				return 6;
+				ret = 6;
+				goto out_free;
 			}
 		}
 
@@ -528,7 +535,8 @@ int git_config_set_multivar(const char* 
 				regfree(store.value_regex);
 				free(store.value_regex);
 			}
-			return 3;
+			ret = 3;
+			goto out_free;
 		}
 
 		free(store.key);
@@ -542,7 +550,8 @@ int git_config_set_multivar(const char* 
 				(store.seen > 1 && multi_replace == 0)) {
 			close(fd);
 			unlink(lock_file);
-			return 5;
+			ret = 5;
+			goto out_free;
 		}
 
 		fstat(in_fd, &st);
@@ -593,10 +602,18 @@ int git_config_set_multivar(const char* 
 
 	if (rename(lock_file, config_filename) < 0) {
 		fprintf(stderr, "Could not rename the lock file?\n");
-		return 4;
+		ret = 4;
+		goto out_free;
 	}
 
-	return 0;
+	ret = 0;
+
+out_free:
+	if (config_filename)
+		free(config_filename);
+	if (lock_file)
+		free(lock_file);
+	return ret;
 }
 
 
-- 
1.2.5

[PATCH 5/7] cleanups: Remove unused variable from sha1_file.c

[Serge E. Hallyn]

Left is assigned to, but never used in sha1_file.c

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 sha1_file.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

0c6ecfcb112a0fb3f1ea51d8e3c220aae235a007
diff --git a/sha1_file.c b/sha1_file.c
index e3d0113..4301c80 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -874,13 +874,12 @@ void packed_object_info_detail(struct pa
 			       unsigned char *base_sha1)
 {
 	struct packed_git *p = e->p;
-	unsigned long offset, left;
+	unsigned long offset;
 	unsigned char *pack;
 	enum object_type kind;
 
 	offset = unpack_object_header(p, e->offset, &kind, size);
 	pack = p->pack_base + offset;
-	left = p->pack_size - offset;
 	if (kind != OBJ_DELTA)
 		*delta_chain_length = 0;
 	else {
-- 
1.2.5

Re: [PATCH 5/7] cleanups: Remove unused variable from sha1_file.c

[Junio C Hamano]

"Serge E. Hallyn" <serue@us.ibm.com> writes:

> Left is assigned to, but never used in sha1_file.c

True, but in this case I suspect it should be used to make sure
we have the 20-byte base_sha1 that follows the header in
deltified case, perhaps like this:

diff --git a/sha1_file.c b/sha1_file.c
index e3d0113..929f481 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -874,17 +874,19 @@ void packed_object_info_detail(struct pa
 			       unsigned char *base_sha1)
 {
 	struct packed_git *p = e->p;
-	unsigned long offset, left;
+	unsigned long offset;
 	unsigned char *pack;
 	enum object_type kind;
 
 	offset = unpack_object_header(p, e->offset, &kind, size);
 	pack = p->pack_base + offset;
-	left = p->pack_size - offset;
 	if (kind != OBJ_DELTA)
 		*delta_chain_length = 0;
 	else {
 		unsigned int chain_length = 0;
+		if (p->pack_size - 20 < offset)
+			die("pack file %s records an incomplete delta base",
+			    p->pack_name);
 		memcpy(base_sha1, pack, 20);
 		do {
 			struct pack_entry base_ent;

[PATCH 4/7] cleanups: Remove impossible case in quote.c

[Serge E. Hallyn]

The switch is inside an if statement which is false if
the character is ' '.  Either the if should be <=' '
instead of <' ', or the case should be removed as it could
be misleading.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 quote.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

86bdfea7f2e88ed01869f370574420a6de3359d7
diff --git a/quote.c b/quote.c
index 7218a70..06792d4 100644
--- a/quote.c
+++ b/quote.c
@@ -144,8 +144,6 @@ static int quote_c_style_counted(const c
 
 			case '\\': /* fallthru */
 			case '"': EMITQ(); break;
-			case ' ':
-				break;
 			default:
 				/* octal */
 				EMITQ();
-- 
1.2.5

[PATCH 7/7] cleanups: remove unused variable from exec_cmd.c

[Serge E. Hallyn]

Not sure whether it should be removed, or whether
execv_git_cmd() should return it rather than -1 at bottom.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 exec_cmd.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

3229a225008ab56b33b1ae7511d91f6b698cd19a
diff --git a/exec_cmd.c b/exec_cmd.c
index 590e738..44bb2f2 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -32,7 +32,7 @@ const char *git_exec_path(void)
 int execv_git_cmd(const char **argv)
 {
 	char git_command[PATH_MAX + 1];
-	int len, err, i;
+	int len,  i;
 	const char *paths[] = { current_exec_path,
 				getenv("GIT_EXEC_PATH"),
 				builtin_exec_path };
@@ -85,8 +85,6 @@ int execv_git_cmd(const char **argv)
 		/* execve() can only ever return if it fails */
 		execve(git_command, (char **)argv, environ);
 
-		err = errno;
-
 		argv[0] = tmp;
 	}
 	return -1;
-- 
1.2.5

Re: [PATCH 7/7] cleanups: remove unused variable from exec_cmd.c

[Junio C Hamano]

"Serge E. Hallyn" <serue@us.ibm.com> writes:

> Not sure whether it should be removed, or whether
> execv_git_cmd() should return it rather than -1 at bottom.

The only one that does not just die() upon failure is git.c but
it checks errno itself, so I think removal is fine.