{"thread":{"id":"3902","subject":"[PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c","startedAt":"2006-04-17T15:14:47Z","lastAt":"2006-04-18T19:32:12Z","messageCount":13,"participants":["Serge E. Hallyn","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"18773","messageId":"20060417151447.D4FE619B90E@sergelap.hallyn.com","threadId":"3902","inReplyTo":"20060417145148.2F3CB19C90D@sergelap.hallyn.com","subject":"[PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c","fromName":"Serge E. Hallyn","fromEmail":"serue@us.ibm.com","sentAt":"2006-04-17T15:14:47Z","receivedAt":"2006-04-17T15:14:47Z","isPatch":true,"sender":{"key":"serue@us.ibm.com","avatar":null},"body":"Address two reports from an automatic code analyzer:\n\n1. In logreport, it is possible to write \\0 one\ncharacter past the end of buf[].\n\n2. In socksetup, socklist can be leaked when returning\nif set_reuse_addr(). Note: dunno why this case returns...\n\nSigned-off-by: Serge E. Hallyn \n\n---\n\n daemon.c | 5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\n5b3e0254d34da582b7593084356c88a923f42a49\ndiff --git a/daemon.c b/daemon.c\nindex a1ccda3..7ac1bc7 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -65,8 +65,8 @@ static void logreport(int priority, cons\n \t * we have space for our own LF and NUL after the \"meat\" of the\n \t * message, so truncate it at maxlen - 1.\n \t */\n-\tif (msglen > maxlen - 1)\n-\t\tmsglen = maxlen - 1;\n+\tif (msglen > maxlen - 2)\n+\t\tmsglen = maxlen - 2;\n \telse if (msglen < 0)\n \t\tmsglen = 0; /* Protect against weird return values. */\n \tbuflen += msglen;\n@@ -535,6 +535,7 @@ static int socksetup(int port, int **soc\n \n \t\tif (set_reuse_addr(sockfd)) {\n \t\t\tclose(sockfd);\n+\t\t\tfree(socklist);\n \t\t\treturn 0;\t/* not fatal */\n \t\t}\n \n-- \n1.2.5\n"},{"id":"18775","messageId":"20060417151447.E2A5919B90F@sergelap.hallyn.com","threadId":"3902","inReplyTo":"20060417145148.2F3CB19C90D@sergelap.hallyn.com","subject":"[PATCH 2/7] cleanups: Fix potential bugs in connect.c","fromName":"Serge E. Hallyn","fromEmail":"serue@us.ibm.com","sentAt":"2006-04-17T15:14:47Z","receivedAt":"2006-04-17T15:14:47Z","isPatch":true,"sender":{"key":"serue@us.ibm.com","avatar":null},"body":"The strncmp for ACK was ACK does not include the final space.\nPresumably either we should either remove the trailing space,\nor compare 4 chars (as this patch does).\n\n'path' is sometimes strdup'ed, but never freed.\n\nSigned-off-by: Serge E. Hallyn \n\n---\n\n connect.c | 18 ++++++++++++++----\n 1 files changed, 14 insertions(+), 4 deletions(-)\n\n1b4f45918d8c1f7d579840c7bb1fe3fe1967b6c5\ndiff --git a/connect.c b/connect.c\nindex 3f2d65c..6a8f8a6 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -74,7 +74,7 @@ int get_ack(int fd, unsigned char *resul\n \t\tline[--len] = 0;\n \tif (!strcmp(line, \"NAK\"))\n \t\treturn 0;\n-\tif (!strncmp(line, \"ACK \", 3)) {\n+\tif (!strncmp(line, \"ACK \", 4)) {\n \t\tif (!get_sha1_hex(line+4, result_sha1)) {\n \t\t\tif (strstr(line+45, \"continue\"))\n \t\t\t\treturn 2;\n@@ -567,6 +567,7 @@ int git_connect(int fd[2], char *url, co\n \tint pipefd[2][2];\n \tpid_t pid;\n \tenum protocol protocol = PROTO_LOCAL;\n+\tint free_path = 0;\n \n \thost = strstr(url, \"://\");\n \tif(host) {\n@@ -610,16 +611,23 @@ int git_connect(int fd[2], char *url, co\n \t\tchar *ptr = path;\n \t\tif (path[1] == '~')\n \t\t\tpath++;\n-\t\telse\n+\t\telse {\n \t\t\tpath = strdup(ptr);\n+\t\t\tfree_path = 1;\n+\t\t}\n \n \t\t*ptr = '\\0';\n \t}\n \n \tif (protocol == PROTO_GIT) {\n+\t\tint ret;\n \t\tif (git_use_proxy(host))\n-\t\t\treturn git_proxy_connect(fd, prog, host, path);\n-\t\treturn git_tcp_connect(fd, prog, host, path);\n+\t\t\tret = git_proxy_connect(fd, prog, host, path);\n+\t\telse\n+\t\t\tret = git_tcp_connect(fd, prog, host, path);\n+\t\tif (free_path)\n+\t\t\tfree(path);\n+\t\treturn ret;\n \t}\n \n \tif (pipe(pipefd[0]) < 0 || pipe(pipefd[1]) < 0)\n@@ -659,6 +667,8 @@ int git_connect(int fd[2], char *url, co\n \tfd[1] = pipefd[1][1];\n \tclose(pipefd[0][1]);\n \tclose(pipefd[1][0]);\n+\tif (free_path)\n+\t\tfree(path);\n \treturn pid;\n }\n \n-- \n1.2.5\n"},{"id":"18778","messageId":"20060417145148.2F3CB19C90D@sergelap.hallyn.com","threadId":"3902","inReplyTo":null,"subject":"[PATCH 0/7] cleanups: intro","fromName":"Serge E. Hallyn","fromEmail":"serue@us.ibm.com","sentAt":"2006-04-17T15:14:47Z","receivedAt":"2006-04-17T15:14:47Z","isPatch":true,"sender":{"key":"serue@us.ibm.com","avatar":null},"body":"While debating whether to run git-daemon at home, I figured I'd\nrun some static analysis on the source. The following patches\nare all pretty trivial, addressing things the tool pointed out.\n\nthanks,\n-serge\n"},{"id":"18779","messageId":"20060417151447.EC97F19B8FD@sergelap.hallyn.com","threadId":"3902","inReplyTo":"20060417145148.2F3CB19C90D@sergelap.hallyn.com","subject":"[PATCH 3/7] cleanups: Remove unused vars from combine-diff.c","fromName":"Serge E. Hallyn","fromEmail":"serue@us.ibm.com","sentAt":"2006-04-17T15:14:47Z","receivedAt":"2006-04-17T15:14:47Z","isPatch":true,"sender":{"key":"serue@us.ibm.com","avatar":null},"body":"Mod_type in particular sure looks like it wants to be used, but isn't.\n\nSigned-off-by: Serge E. Hallyn \n\n---\n\n combine-diff.c | 12 ++----------\n 1 files changed, 2 insertions(+), 10 deletions(-)\n\n5a7c4023402dd2420a4596cdd92a1a46fd3e5c14\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 9bd27f8..9445e86 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -589,7 +589,7 @@ static int show_patch_diff(struct combin\n \t\t\t struct diff_options *opt)\n {\n \tunsigned long result_size, cnt, lno;\n-\tchar *result, *cp, *ep;\n+\tchar *result, *cp;\n \tstruct sline *sline; /* survived lines */\n \tint mode_differs = 0;\n \tint i, show_hunks, shown_header = 0;\n@@ -641,7 +641,6 @@ static int show_patch_diff(struct combin\n \t\tcnt++; /* incomplete line */\n \n \tsline = xcalloc(cnt+2, sizeof(*sline));\n-\tep = result;\n \tsline[0].bol = result;\n \tfor (lno = 0; lno <= cnt + 1; lno++) {\n \t\tsline[lno].lost_tail = &sline[lno].lost_head;\n@@ -752,7 +751,7 @@ static int show_patch_diff(struct combin\n \n static void show_raw_diff(struct combine_diff_path *p, int num_parent, const char *header, struct diff_options *opt)\n {\n-\tint i, offset, mod_type = 'A';\n+\tint i, offset;\n \tconst char *prefix;\n \tint line_termination, inter_name_termination;\n \n@@ -764,13 +763,6 @@ static void show_raw_diff(struct combine\n \tif (header)\n \t\tprintf(\"%s%c\", header, line_termination);\n \n-\tfor (i = 0; i < num_parent; i++) {\n-\t\tif (p->parent[i].mode)\n-\t\t\tmod_type = 'M';\n-\t}\n-\tif (!p->mode)\n-\t\tmod_type = 'D';\n-\n \tif (opt->output_format == DIFF_FORMAT_RAW) {\n \t\toffset = strlen(COLONS) - num_parent;\n \t\tif (offset < 0)\n-- \n1.2.5\n"},{"id":"18774","messageId":"20060417151448.1D99C19B912@sergelap.hallyn.com","threadId":"3902","inReplyTo":"20060417145148.2F3CB19C90D@sergelap.hallyn.com","subject":"[PATCH 5/7] cleanups: Remove unused variable from sha1_file.c","fromName":"Serge E. Hallyn","fromEmail":"serue@us.ibm.com","sentAt":"2006-04-17T15:14:48Z","receivedAt":"2006-04-17T15:14:48Z","isPatch":true,"sender":{"key":"serue@us.ibm.com","avatar":null},"body":"Left is assigned to, but never used in sha1_file.c\n\nSigned-off-by: Serge E. Hallyn \n\n---\n\n sha1_file.c | 3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\n0c6ecfcb112a0fb3f1ea51d8e3c220aae235a007\ndiff --git a/sha1_file.c b/sha1_file.c\nindex e3d0113..4301c80 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -874,13 +874,12 @@ void packed_object_info_detail(struct pa\n \t\t\t unsigned char *base_sha1)\n {\n \tstruct packed_git *p = e->p;\n-\tunsigned long offset, left;\n+\tunsigned long offset;\n \tunsigned char *pack;\n \tenum object_type kind;\n \n \toffset = unpack_object_header(p, e->offset, &kind, size);\n \tpack = p->pack_base + offset;\n-\tleft = p->pack_size - offset;\n \tif (kind != OBJ_DELTA)\n \t\t*delta_chain_length = 0;\n \telse {\n-- \n1.2.5\n"},{"id":"18776","messageId":"20060417151448.271D719B913@sergelap.hallyn.com","threadId":"3902","inReplyTo":"20060417145148.2F3CB19C90D@sergelap.hallyn.com","subject":"[PATCH 6/7] cleanups: prevent leak of two strduped strings in config.c","fromName":"Serge E. Hallyn","fromEmail":"serue@us.ibm.com","sentAt":"2006-04-17T15:14:48Z","receivedAt":"2006-04-17T15:14:48Z","isPatch":true,"sender":{"key":"serue@us.ibm.com","avatar":null},"body":"Config_filename and lockfile are strduped and then leaked in\ngit_config_set_multivar.\n\nSigned-off-by: Serge E. Hallyn \n\n---\n\n config.c | 37 +++++++++++++++++++++++++++----------\n 1 files changed, 27 insertions(+), 10 deletions(-)\n\nc96b9a1a358f8929a2525dd4e15551d286aec361\ndiff --git a/config.c b/config.c\nindex 95ec349..a37bb2d 100644\n--- a/config.c\n+++ b/config.c\n@@ -420,6 +420,7 @@ int git_config_set_multivar(const char* \n {\n \tint i;\n \tint fd, in_fd;\n+\tint ret;\n \tchar* config_filename = strdup(git_path(\"config\"));\n \tchar* lock_file = strdup(git_path(\"config.lock\"));\n \tconst char* last_dot = strrchr(key, '.');\n@@ -431,7 +432,8 @@ int git_config_set_multivar(const char* \n \n \tif (last_dot == NULL) {\t\n \t\tfprintf(stderr, \"key does not contain a section: %s\\n\", key);\n-\t\treturn 2;\n+\t\tret = 2;\n+\t\tgoto out_free;\n \t}\n \tstore.baselen = last_dot - key;\n \n@@ -447,7 +449,8 @@ int git_config_set_multivar(const char* \n \t\t\t\t (i == store.baselen+1 && !isalpha(key[i])))) {\n \t\t\tfprintf(stderr, \"invalid key: %s\\n\", key);\n \t\t\tfree(store.key);\n-\t\t\treturn 1;\n+\t\t\tret = 1;\n+\t\t\tgoto out_free;\n \t\t} else\n \t\t\tstore.key[i] = tolower(key[i]);\n \tstore.key[i] = 0;\n@@ -460,7 +463,8 @@ int git_config_set_multivar(const char* \n \tif (fd < 0) {\n \t\tfprintf(stderr, \"could not lock config file\\n\");\n \t\tfree(store.key);\n-\t\treturn -1;\n+\t\tret = -1;\n+\t\tgoto out_free;\n \t}\n \n \t/*\n@@ -475,13 +479,15 @@ int git_config_set_multivar(const char* \n \t\t\t strerror(errno));\n \t\t\tclose(fd);\n \t\t\tunlink(lock_file);\n-\t\t\treturn 3; /* same as \"invalid config file\" */\n+\t\t\tret = 3; /* same as \"invalid config file\" */\n+\t\t\tgoto out_free;\n \t\t}\n \t\t/* if nothing to unset, error out */\n \t\tif (value == NULL) {\n \t\t\tclose(fd);\n \t\t\tunlink(lock_file);\n-\t\t\treturn 5;\n+\t\t\tret = 5;\n+\t\t\tgoto out_free;\n \t\t}\n \n \t\tstore.key = (char*)key;\n@@ -507,7 +513,8 @@ int git_config_set_multivar(const char* \n \t\t\t\tfprintf(stderr, \"Invalid pattern: %s\\n\",\n \t\t\t\t\tvalue_regex);\n \t\t\t\tfree(store.value_regex);\n-\t\t\t\treturn 6;\n+\t\t\t\tret = 6;\n+\t\t\t\tgoto out_free;\n \t\t\t}\n \t\t}\n \n@@ -528,7 +535,8 @@ int git_config_set_multivar(const char* \n \t\t\t\tregfree(store.value_regex);\n \t\t\t\tfree(store.value_regex);\n \t\t\t}\n-\t\t\treturn 3;\n+\t\t\tret = 3;\n+\t\t\tgoto out_free;\n \t\t}\n \n \t\tfree(store.key);\n@@ -542,7 +550,8 @@ int git_config_set_multivar(const char* \n \t\t\t\t(store.seen > 1 && multi_replace == 0)) {\n \t\t\tclose(fd);\n \t\t\tunlink(lock_file);\n-\t\t\treturn 5;\n+\t\t\tret = 5;\n+\t\t\tgoto out_free;\n \t\t}\n \n \t\tfstat(in_fd, &st);\n@@ -593,10 +602,18 @@ int git_config_set_multivar(const char* \n \n \tif (rename(lock_file, config_filename) < 0) {\n \t\tfprintf(stderr, \"Could not rename the lock file?\\n\");\n-\t\treturn 4;\n+\t\tret = 4;\n+\t\tgoto out_free;\n \t}\n \n-\treturn 0;\n+\tret = 0;\n+\n+out_free:\n+\tif (config_filename)\n+\t\tfree(config_filename);\n+\tif (lock_file)\n+\t\tfree(lock_file);\n+\treturn ret;\n }\n \n \n-- \n1.2.5\n"},{"id":"18777","messageId":"20060417151448.0EE0F19B911@sergelap.hallyn.com","threadId":"3902","inReplyTo":"20060417145148.2F3CB19C90D@sergelap.hallyn.com","subject":"[PATCH 4/7] cleanups: Remove impossible case in quote.c","fromName":"Serge E. Hallyn","fromEmail":"serue@us.ibm.com","sentAt":"2006-04-17T15:14:48Z","receivedAt":"2006-04-17T15:14:48Z","isPatch":true,"sender":{"key":"serue@us.ibm.com","avatar":null},"body":"The switch is inside an if statement which is false if\nthe character is ' '. Either the if should be <=' '\ninstead of <' ', or the case should be removed as it could\nbe misleading.\n\nSigned-off-by: Serge E. Hallyn \n\n---\n\n quote.c | 2 --\n 1 files changed, 0 insertions(+), 2 deletions(-)\n\n86bdfea7f2e88ed01869f370574420a6de3359d7\ndiff --git a/quote.c b/quote.c\nindex 7218a70..06792d4 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -144,8 +144,6 @@ static int quote_c_style_counted(const c\n \n \t\t\tcase '\\\\': /* fallthru */\n \t\t\tcase '\"': EMITQ(); break;\n-\t\t\tcase ' ':\n-\t\t\t\tbreak;\n \t\t\tdefault:\n \t\t\t\t/* octal */\n \t\t\t\tEMITQ();\n-- \n1.2.5\n"},{"id":"18780","messageId":"20060417151448.3130F19B914@sergelap.hallyn.com","threadId":"3902","inReplyTo":"20060417145148.2F3CB19C90D@sergelap.hallyn.com","subject":"[PATCH 7/7] cleanups: remove unused variable from exec_cmd.c","fromName":"Serge E. Hallyn","fromEmail":"serue@us.ibm.com","sentAt":"2006-04-17T15:14:48Z","receivedAt":"2006-04-17T15:14:48Z","isPatch":true,"sender":{"key":"serue@us.ibm.com","avatar":null},"body":"Not sure whether it should be removed, or whether\nexecv_git_cmd() should return it rather than -1 at bottom.\n\nSigned-off-by: Serge E. Hallyn \n\n---\n\n exec_cmd.c | 4 +---\n 1 files changed, 1 insertions(+), 3 deletions(-)\n\n3229a225008ab56b33b1ae7511d91f6b698cd19a\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 590e738..44bb2f2 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -32,7 +32,7 @@ const char *git_exec_path(void)\n int execv_git_cmd(const char **argv)\n {\n \tchar git_command[PATH_MAX + 1];\n-\tint len, err, i;\n+\tint len, i;\n \tconst char *paths[] = { current_exec_path,\n \t\t\t\tgetenv(\"GIT_EXEC_PATH\"),\n \t\t\t\tbuiltin_exec_path };\n@@ -85,8 +85,6 @@ int execv_git_cmd(const char **argv)\n \t\t/* execve() can only ever return if it fails */\n \t\texecve(git_command, (char **)argv, environ);\n \n-\t\terr = errno;\n-\n \t\targv[0] = tmp;\n \t}\n \treturn -1;\n-- \n1.2.5\n"},{"id":"18787","messageId":"7v64l7lw04.fsf@assigned-by-dhcp.cox.net","threadId":"3902","inReplyTo":"20060417151448.1D99C19B912@sergelap.hallyn.com","subject":"Re: [PATCH 5/7] cleanups: Remove unused variable from sha1_file.c","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-17T21:42:19Z","receivedAt":"2006-04-17T21:42:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Serge E. Hallyn\" writes:\n\n> Left is assigned to, but never used in sha1_file.c\n\nTrue, but in this case I suspect it should be used to make sure\nwe have the 20-byte base_sha1 that follows the header in\ndeltified case, perhaps like this:\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex e3d0113..929f481 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -874,17 +874,19 @@ void packed_object_info_detail(struct pa\n \t\t\t unsigned char *base_sha1)\n {\n \tstruct packed_git *p = e->p;\n-\tunsigned long offset, left;\n+\tunsigned long offset;\n \tunsigned char *pack;\n \tenum object_type kind;\n \n \toffset = unpack_object_header(p, e->offset, &kind, size);\n \tpack = p->pack_base + offset;\n-\tleft = p->pack_size - offset;\n \tif (kind != OBJ_DELTA)\n \t\t*delta_chain_length = 0;\n \telse {\n \t\tunsigned int chain_length = 0;\n+\t\tif (p->pack_size - 20 < offset)\n+\t\t\tdie(\"pack file %s records an incomplete delta base\",\n+\t\t\t p->pack_name);\n \t\tmemcpy(base_sha1, pack, 20);\n \t\tdo {\n \t\t\tstruct pack_entry base_ent;\n"},{"id":"18788","messageId":"7vzmijkhf8.fsf@assigned-by-dhcp.cox.net","threadId":"3902","inReplyTo":"20060417151448.3130F19B914@sergelap.hallyn.com","subject":"Re: [PATCH 7/7] cleanups: remove unused variable from exec_cmd.c","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-17T21:42:35Z","receivedAt":"2006-04-17T21:42:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Serge E. Hallyn\" writes:\n\n> Not sure whether it should be removed, or whether\n> execv_git_cmd() should return it rather than -1 at bottom.\n\nThe only one that does not just die() upon failure is git.c but\nit checks errno itself, so I think removal is fine.\n"},{"id":"18789","messageId":"7vu08rkhey.fsf@assigned-by-dhcp.cox.net","threadId":"3902","inReplyTo":"20060417151447.D4FE619B90E@sergelap.hallyn.com","subject":"Re: [PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-17T21:42:45Z","receivedAt":"2006-04-17T21:42:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Serge E. Hallyn\" writes:\n\n> Address two reports from an automatic code analyzer:\n>\n> 1. In logreport, it is possible to write \\0 one\n> character past the end of buf[].\n\nI am perhaps slower than I usually am today, but it seems to me\nthat the code caps msglen to (maxlen-1) and then adds that to\nbuflen.\n\nNow, maxlen is (sizeof(buf)-buflen-1), so that means after\nthe \"buflen += msglen\" happens, buflen is at most:\n\n\tbuflen + (sizeof(buf)-buflen-1) - 1\n = sizeof(buf) - 2\n\nAnd then \"buf[buflen++] = '\\n'; buf[buflen] = '\\0'\" happens.\n'\\n' is written at sizeof(buf)-2 (or lower index than that) and\n'\\0' is written at sizeof(buf)-1 (or lower). I am unsure how it\nsteps beyond the end...\n\n> 2. In socksetup, socklist can be leaked when returning\n> if set_reuse_addr(). Note: dunno why this case returns...\n\nI am not sure why this part returns either. It appears to me\nthat it should just keep going just like the cases where\nbind/listen fails.\n"},{"id":"18802","messageId":"20060418131106.GD7562@sergelap.austin.ibm.com","threadId":"3902","inReplyTo":"7vu08rkhey.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c","fromName":"Serge E. Hallyn","fromEmail":"serue@us.ibm.com","sentAt":"2006-04-18T13:11:06Z","receivedAt":"2006-04-18T13:11:06Z","isPatch":true,"sender":{"key":"serue@us.ibm.com","avatar":null},"body":"Quoting Junio C Hamano (junkio@cox.net):\n> \"Serge E. Hallyn\" writes:\n> \n> > Address two reports from an automatic code analyzer:\n> >\n> > 1. In logreport, it is possible to write \\0 one\n> > character past the end of buf[].\n> \n> I am perhaps slower than I usually am today, but it seems to me\n> that the code caps msglen to (maxlen-1) and then adds that to\n> buflen.\n> \n> Now, maxlen is (sizeof(buf)-buflen-1), so that means after\n> the \"buflen += msglen\" happens, buflen is at most:\n> \n> \tbuflen + (sizeof(buf)-buflen-1) - 1\n> = sizeof(buf) - 2\n> \n> And then \"buf[buflen++] = '\\n'; buf[buflen] = '\\0'\" happens.\n> '\\n' is written at sizeof(buf)-2 (or lower index than that) and\n> '\\0' is written at sizeof(buf)-1 (or lower). I am unsure how it\n> steps beyond the end...\n\nArgh, I had to pull out a sheet of paper, but you are right. I\nmisread, and the warning must be about the case where the\nsnprint \"[%ld] \" prints out 1023 characters.\n\n> > 2. In socksetup, socklist can be leaked when returning\n> > if set_reuse_addr(). Note: dunno why this case returns...\n> \n> I am not sure why this part returns either. It appears to me\n> that it should just keep going just like the cases where\n> bind/listen fails.\n\nThen perhaps the following is more appropriate.\n\nthanks,\n-serge\n\nFrom: Serge E. Hallyn \nSubject: [PATCH] socksetup: don't return on set_reuse_addr() error\n\nThe set_reuse_addr() error case was the only error case in\nsocklist() where we returned rather than continued. Not sure\nwhy. Either we must free the socklist, or continue. This patch\ncontinues on error.\n\nSigned-off-by: Serge E. Hallyn \n\n---\n\n daemon.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\nb589029e3187eed51c3fe6a2715f51bea2159786\ndiff --git a/daemon.c b/daemon.c\nindex a1ccda3..776749e 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -535,7 +535,7 @@ static int socksetup(int port, int **soc\n \n \t\tif (set_reuse_addr(sockfd)) {\n \t\t\tclose(sockfd);\n-\t\t\treturn 0;\t/* not fatal */\n+\t\t\tcontinue;\n \t\t}\n \n \t\tif (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {\n-- \n1.2.5\n"},{"id":"18824","messageId":"7vzmiiel37.fsf@assigned-by-dhcp.cox.net","threadId":"3902","inReplyTo":"20060418131106.GD7562@sergelap.austin.ibm.com","subject":"Re: [PATCH 1/7] cleanups: Fix resource leak and buffer overrun in daemon.c","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-18T19:32:12Z","receivedAt":"2006-04-18T19:32:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Serge E. Hallyn\" writes:\n\n> Argh, I had to pull out a sheet of paper, but you are right. I\n> misread, and the warning must be about the case where the\n> snprint \"[%ld] \" prints out 1023 characters.\n\nIf snprintf(buf, sizeof(buf), \"[%ld] \", (long) getpid()) is\njudged to possibly overrun the buffer by your static analysis\ntool, I think the tool is broken. It at least should know how\nbig a printed long can be. It would earn bonus points if it can\nwarn me when sizeof(buf) is sufficiently small, say 40 bytes,\nwith a message like \"on future architectures with 128-bit long\nthis code might break\" ;-).\n"}]}