{"thread":{"id":"37203","subject":"[PATCH 0/5] coverity mixed bag","startedAt":"2014-07-24T04:39:41Z","lastAt":"2014-07-29T05:36:44Z","messageCount":8,"participants":["Jeff King","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"246626","messageId":"20140724043940.GA31282@peff.net","threadId":"37203","inReplyTo":null,"subject":"[PATCH 0/5] coverity mixed bag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-24T04:39:41Z","receivedAt":"2014-07-24T04:39:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since Stefan has recently started feeding git builds to coverity, I\nspent a few minutes poking through the results. There are tons of false\npositives, so there is some work to be done there with tweaking our\ncoverity models. But there are some real issues, too. Here are fixes for\nthe handful that I looked at.\n\n [1/5]: receive-pack: don't copy \"dir\" parameter\n [2/5]: free ref string returned by dwim_ref\n [3/5]: transport: fix leaks in refs_from_alternate_cb\n [4/5]: fix memory leak parsing core.commentchar\n [5/5]: apply: avoid possible bogus pointer\n\n-Peff\n"},{"id":"246627","messageId":"20140724044042.GA32355@peff.net","threadId":"37203","inReplyTo":"20140724043940.GA31282@peff.net","subject":"[PATCH 1/5] receive-pack: don't copy \"dir\" parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-24T04:40:43Z","receivedAt":"2014-07-24T04:40:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We used to do this so could pass a mutable string to\nenter_repo. But since 1c64b48 (enter_repo: do not modify\ninput, 2011-10-04), this is not necessary.\n\nThe resulting code is simpler, and it fixes a minor leak.\n\nSigned-off-by: Jeff King \n---\nIf you are wondering whether upload-pack needs the same treatment, we\nalready did it in 6379dd0.\n\n builtin/receive-pack.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 92561bf..f93ac45 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1122,7 +1122,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \tint advertise_refs = 0;\n \tint stateless_rpc = 0;\n \tint i;\n-\tchar *dir = NULL;\n+\tconst char *dir = NULL;\n \tstruct command *commands;\n \tstruct sha1_array shallow = SHA1_ARRAY_INIT;\n \tstruct sha1_array ref = SHA1_ARRAY_INIT;\n@@ -1157,7 +1157,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (dir)\n \t\t\tusage(receive_pack_usage);\n-\t\tdir = xstrdup(arg);\n+\t\tdir = arg;\n \t}\n \tif (!dir)\n \t\tusage(receive_pack_usage);\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"246628","messageId":"20140724044111.GB32355@peff.net","threadId":"37203","inReplyTo":"20140724043940.GA31282@peff.net","subject":"[PATCH 2/5] free ref string returned by dwim_ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-24T04:41:11Z","receivedAt":"2014-07-24T04:41:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A call to \"dwim_ref(name, len, flags, &ref)\" will allocate a\nnew string in \"ref\" to return the exact ref we found. We do\nnot consistently free it in all code paths, leading to small\nleaks. The worst is in get_sha1_basic, which may be called\nmany times (e.g., by \"cat-file --batch\"), though it is\nrelatively unlikely, as it only triggers on a bogus reflog\nspecification.\n\nSigned-off-by: Jeff King \n---\n builtin/rev-parse.c | 1 +\n builtin/show-branch.c | 1 +\n sha1_name.c | 4 +++-\n 3 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 8102aaa..d85e08c 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -151,6 +151,7 @@ static void show_rev(int type, const unsigned char *sha1, const char *name)\n \t\t\t\terror(\"refname '%s' is ambiguous\", name);\n \t\t\t\tbreak;\n \t\t\t}\n+\t\t\tfree(full);\n \t\t} else {\n \t\t\tshow_with_type(type, name);\n \t\t}\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 5fd4e4e..298c95e 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -777,6 +777,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\tsprintf(nth_desc, \"%s@{%d}\", *av, base+i);\n \t\t\tappend_ref(nth_desc, sha1, 1);\n \t\t}\n+\t\tfree(ref);\n \t}\n \telse if (all_heads + all_remotes)\n \t\tsnarf_refs(all_heads, all_remotes);\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 6ccd3a5..63ee66f 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -540,8 +540,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\t\tchar *tmp = xstrndup(str + at + 2, reflog_len);\n \t\t\tat_time = approxidate_careful(tmp, &errors);\n \t\t\tfree(tmp);\n-\t\t\tif (errors)\n+\t\t\tif (errors) {\n+\t\t\t\tfree(real_ref);\n \t\t\t\treturn -1;\n+\t\t\t}\n \t\t}\n \t\tif (read_ref_at(real_ref, at_time, nth, sha1, NULL,\n \t\t\t\t&co_time, &co_tz, &co_cnt)) {\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"246629","messageId":"20140724044130.GC32355@peff.net","threadId":"37203","inReplyTo":"20140724043940.GA31282@peff.net","subject":"[PATCH 3/5] transport: fix leaks in refs_from_alternate_cb","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-24T04:41:30Z","receivedAt":"2014-07-24T04:41:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The function starts by creating a copy of the static buffer\nreturned by real_path, but forgets to free it in the error\ncode paths. We can solve this by jumping to the cleanup code\nthat is already there.\n\nSigned-off-by: Jeff King \n---\n transport.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 3e42570..297e981 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1359,11 +1359,11 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,\n \twhile (other[len-1] == '/')\n \t\tother[--len] = '\\0';\n \tif (len < 8 || memcmp(other + len - 8, \"/objects\", 8))\n-\t\treturn 0;\n+\t\tgoto out;\n \t/* Is this a git repository with refs? */\n \tmemcpy(other + len - 8, \"/refs\", 6);\n \tif (!is_directory(other))\n-\t\treturn 0;\n+\t\tgoto out;\n \tother[len - 8] = '\\0';\n \tremote = remote_get(other);\n \ttransport = transport_get(remote, other);\n@@ -1372,6 +1372,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,\n \t extra = extra->next)\n \t\tcb->fn(extra, cb->data);\n \ttransport_disconnect(transport);\n+out:\n \tfree(other);\n \treturn 0;\n }\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"246630","messageId":"20140724044239.GD32355@peff.net","threadId":"37203","inReplyTo":"20140724043940.GA31282@peff.net","subject":"[PATCH 4/5] fix memory leak parsing core.commentchar","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-24T04:42:39Z","receivedAt":"2014-07-24T04:42:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we see the core.commentchar config option, we extract\nthe string with git_config_string, which does two things:\n\n 1. It complains via config_error_nonbool if there is no\n string value.\n\n 2. It makes a copy of the string.\n\nSince we immediately parse the string into its\nsingle-character value, we only care about (1). And in fact\n(2) is a detriment, as it means we leak the copy. Instead,\nlet's just check the pointer value ourselves, and parse\ndirectly from the const string we already have.\n\nSigned-off-by: Jeff King \n---\n config.c | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 9767c4b..058505c 100644\n--- a/config.c\n+++ b/config.c\n@@ -817,14 +817,12 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn git_config_string(&editor_program, var, value);\n \n \tif (!strcmp(var, \"core.commentchar\")) {\n-\t\tconst char *comment;\n-\t\tint ret = git_config_string(&comment, var, value);\n-\t\tif (ret)\n-\t\t\treturn ret;\n-\t\telse if (!strcasecmp(comment, \"auto\"))\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\telse if (!strcasecmp(value, \"auto\"))\n \t\t\tauto_comment_line_char = 1;\n-\t\telse if (comment[0] && !comment[1]) {\n-\t\t\tcomment_line_char = comment[0];\n+\t\telse if (value[0] && !value[1]) {\n+\t\t\tcomment_line_char = value[0];\n \t\t\tauto_comment_line_char = 0;\n \t\t} else\n \t\t\treturn error(\"core.commentChar should only be one character\");\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"246631","messageId":"20140724044323.GE32355@peff.net","threadId":"37203","inReplyTo":"20140724043940.GA31282@peff.net","subject":"[PATCH 5/5] apply: avoid possible bogus pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-24T04:43:23Z","receivedAt":"2014-07-24T04:43:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When parsing \"index\" lines from a git-diff, we look for a\nspace followed by the mode. If we don't have a space, then\nwe set our pointer to the end-of-line. However, we don't\ndouble-check that our end-of-line pointer is valid (e.g., if\nwe got a truncated diff input), which could lead to some\nwrap-around pointer arithmetic.\n\nIn most cases this would probably get caught by our \"40 <\nlen\" check later in the function, but to be on the safe\nside, let's just use strchrnul to treat end-of-string the\nsame as end-of-line.\n\nSigned-off-by: Jeff King \n---\n builtin/apply.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 9f8f5ba..be2b4ce 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1075,7 +1075,7 @@ static int gitdiff_index(const char *line, struct patch *patch)\n \n \tline = ptr + 2;\n \tptr = strchr(line, ' ');\n-\teol = strchr(line, '\\n');\n+\teol = strchrnul(line, '\\n');\n \n \tif (!ptr || eol < ptr)\n \t\tptr = eol;\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"246667","messageId":"xmqqd2cu8pkw.fsf@gitster.dls.corp.google.com","threadId":"37203","inReplyTo":"20140724043940.GA31282@peff.net","subject":"Re: [PATCH 0/5] coverity mixed bag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-24T20:33:03Z","receivedAt":"2014-07-24T20:33:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Since Stefan has recently started feeding git builds to coverity, I\n> spent a few minutes poking through the results. There are tons of false\n> positives, so there is some work to be done there with tweaking our\n> coverity models. But there are some real issues, too. Here are fixes for\n> the handful that I looked at.\n>\n> [1/5]: receive-pack: don't copy \"dir\" parameter\n> [2/5]: free ref string returned by dwim_ref\n> [3/5]: transport: fix leaks in refs_from_alternate_cb\n> [4/5]: fix memory leak parsing core.commentchar\n> [5/5]: apply: avoid possible bogus pointer\n>\n> -Peff\n\nThanks, they all make sense to me. I'd probably have to wiggle 4/5\na bit to port the fix over to both maint and master, though.\n"},{"id":"246871","messageId":"53D732EC.8010302@gmail.com","threadId":"37203","inReplyTo":"20140724043940.GA31282@peff.net","subject":"Re: [PATCH 0/5] coverity mixed bag","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-07-29T05:36:44Z","receivedAt":"2014-07-29T05:36:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 24.07.2014 06:39, Jeff King wrote:\n> Since Stefan has recently started feeding git builds to coverity, I\n> spent a few minutes poking through the results. There are tons of false\n> positives, so there is some work to be done there with tweaking our\n> coverity models. But there are some real issues, too. Here are fixes for\n> the handful that I looked at.\n> \n> [1/5]: receive-pack: don't copy \"dir\" parameter\n> [2/5]: free ref string returned by dwim_ref\n> [3/5]: transport: fix leaks in refs_from_alternate_cb\n> [4/5]: fix memory leak parsing core.commentchar\n> [5/5]: apply: avoid possible bogus pointer\n> \n> -Peff\n> \n\nAs this patchset is in, we have coverity scan now finding\n10 defects fixed.\n\nThanks,\nStefan\n"}]}