[PATCH 0/7] cleanups: intro
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
threads / patch / 3902
patch, 7 partscleanups: Fix resource leak and buffer overrun in daemon.c
Subject: [PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c
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
Show changes to daemon.c +3 −2
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
Junio C HamanoRe: [PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c
"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) - 2And 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
Quoting Junio C Hamano (junkio@cox.net):
Show 21 quoted lines
> "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.
Show 6 quoted lines
> > 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
Show changes to daemon.c +1 −1
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
Junio C HamanoRe: [PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c
"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 2/7] cleanups: Fix potential bugs in connect.c
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
Show changes to connect.c +14 −4
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
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
Show changes to combine-diff.c +2 −10
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 5/7] cleanups: Remove unused variable from sha1_file.c
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
Show changes to sha1_file.c +1 −2
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
Junio C HamanoRe: [PATCH 5/7] cleanups: Remove unused variable from sha1_file.c
"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:
Show changes to sha1_file.c +4 −2
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 6/7] cleanups: prevent leak of two strduped strings in config.c
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
Show changes to config.c +27 −10
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 4/7] cleanups: Remove impossible case in quote.c
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
Show changes to quote.c +0 −2
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
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
Show changes to exec_cmd.c +1 −3
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
Junio C HamanoRe: [PATCH 7/7] cleanups: remove unused variable from exec_cmd.c
"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.