# [PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c 13 messages from 2006-04-17 to 2006-04-18. Participants: Serge E. Hallyn, Junio C Hamano. Thread: https://gitlist.dev/t/3902 ## Serge E. Hallyn, 2006-04-17 15:14 Subject: [PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c Message-ID: <20060417151447.D4FE619B90E@sergelap.hallyn.com> URL: https://gitlist.dev/e/20060417151447.D4FE619B90E%40sergelap.hallyn.com In-Reply-To: <20060417145148.2F3CB19C90D@sergelap.hallyn.com> ``` 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 --- 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 ``` ## Serge E. Hallyn, 2006-04-17 15:14 Subject: [PATCH 2/7] cleanups: Fix potential bugs in connect.c Message-ID: <20060417151447.E2A5919B90F@sergelap.hallyn.com> URL: https://gitlist.dev/e/20060417151447.E2A5919B90F%40sergelap.hallyn.com In-Reply-To: <20060417145148.2F3CB19C90D@sergelap.hallyn.com> ``` 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 --- 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 ``` ## Serge E. Hallyn, 2006-04-17 15:14 Subject: [PATCH 0/7] cleanups: intro Message-ID: <20060417145148.2F3CB19C90D@sergelap.hallyn.com> URL: https://gitlist.dev/e/20060417145148.2F3CB19C90D%40sergelap.hallyn.com ``` 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 ``` ## Serge E. Hallyn, 2006-04-17 15:14 Subject: [PATCH 3/7] cleanups: Remove unused vars from combine-diff.c Message-ID: <20060417151447.EC97F19B8FD@sergelap.hallyn.com> URL: https://gitlist.dev/e/20060417151447.EC97F19B8FD%40sergelap.hallyn.com In-Reply-To: <20060417145148.2F3CB19C90D@sergelap.hallyn.com> ``` Mod_type in particular sure looks like it wants to be used, but isn't. Signed-off-by: Serge E. Hallyn --- 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 ``` ## Serge E. Hallyn, 2006-04-17 15:14 Subject: [PATCH 5/7] cleanups: Remove unused variable from sha1_file.c Message-ID: <20060417151448.1D99C19B912@sergelap.hallyn.com> URL: https://gitlist.dev/e/20060417151448.1D99C19B912%40sergelap.hallyn.com In-Reply-To: <20060417145148.2F3CB19C90D@sergelap.hallyn.com> ``` Left is assigned to, but never used in sha1_file.c Signed-off-by: Serge E. Hallyn --- 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 ``` ## Serge E. Hallyn, 2006-04-17 15:14 Subject: [PATCH 6/7] cleanups: prevent leak of two strduped strings in config.c Message-ID: <20060417151448.271D719B913@sergelap.hallyn.com> URL: https://gitlist.dev/e/20060417151448.271D719B913%40sergelap.hallyn.com In-Reply-To: <20060417145148.2F3CB19C90D@sergelap.hallyn.com> ``` Config_filename and lockfile are strduped and then leaked in git_config_set_multivar. Signed-off-by: Serge E. Hallyn --- 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 ``` ## Serge E. Hallyn, 2006-04-17 15:14 Subject: [PATCH 4/7] cleanups: Remove impossible case in quote.c Message-ID: <20060417151448.0EE0F19B911@sergelap.hallyn.com> URL: https://gitlist.dev/e/20060417151448.0EE0F19B911%40sergelap.hallyn.com In-Reply-To: <20060417145148.2F3CB19C90D@sergelap.hallyn.com> ``` 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 --- 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 ``` ## Serge E. Hallyn, 2006-04-17 15:14 Subject: [PATCH 7/7] cleanups: remove unused variable from exec_cmd.c Message-ID: <20060417151448.3130F19B914@sergelap.hallyn.com> URL: https://gitlist.dev/e/20060417151448.3130F19B914%40sergelap.hallyn.com In-Reply-To: <20060417145148.2F3CB19C90D@sergelap.hallyn.com> ``` 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 --- 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 ``` ## Junio C Hamano, 2006-04-17 21:42 Subject: Re: [PATCH 5/7] cleanups: Remove unused variable from sha1_file.c Message-ID: <7v64l7lw04.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v64l7lw04.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20060417151448.1D99C19B912@sergelap.hallyn.com> ``` "Serge E. Hallyn" 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; ``` ## Junio C Hamano, 2006-04-17 21:42 Subject: Re: [PATCH 7/7] cleanups: remove unused variable from exec_cmd.c Message-ID: <7vzmijkhf8.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vzmijkhf8.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20060417151448.3130F19B914@sergelap.hallyn.com> ``` "Serge E. Hallyn" 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. ``` ## Junio C Hamano, 2006-04-17 21:42 Subject: Re: [PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c Message-ID: <7vu08rkhey.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vu08rkhey.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20060417151447.D4FE619B90E@sergelap.hallyn.com> ``` "Serge E. Hallyn" 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. ``` ## Serge E. Hallyn, 2006-04-18 13:11 Subject: Re: [PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c Message-ID: <20060418131106.GD7562@sergelap.austin.ibm.com> URL: https://gitlist.dev/e/20060418131106.GD7562%40sergelap.austin.ibm.com In-Reply-To: <7vu08rkhey.fsf@assigned-by-dhcp.cox.net> ``` Quoting Junio C Hamano (junkio@cox.net): > "Serge E. Hallyn" 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 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 --- 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 ``` ## Junio C Hamano, 2006-04-18 19:32 Subject: Re: [PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c Message-ID: <7vzmiiel37.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vzmiiel37.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20060418131106.GD7562@sergelap.austin.ibm.com> ``` "Serge E. Hallyn" 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" ;-). ```