coverity mixed bag
cover coverity mixed bag
Since Stefan has recently started feeding git builds to coverity, I spent a few minutes poking through the results. There are tons of false positives, so there is some work to be done there with tweaking our coverity models. But there are some real issues, too. Here are fixes for the handful that I looked at.
[1/5]: receive-pack: don't copy "dir" parameter [2/5]: free ref string returned by dwim_ref [3/5]: transport: fix leaks in refs_from_alternate_cb [4/5]: fix memory leak parsing core.commentchar [5/5]: apply: avoid possible bogus pointer
-Peff
PATCH 1/5 receive-pack: don't copy "dir" parameter
We used to do this so could pass a mutable string to enter_repo. But since 1c64b48 (enter_repo: do not modify input, 2011-10-04), this is not necessary.
The resulting code is simpler, and it fixes a minor leak.
---
If you are wondering whether upload-pack needs the same treatment, we already did it in 6379dd0.
builtin/receive-pack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/receive-pack.c +2 -2
@@ -1122,7 +1122,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)int advertise_refs =0 ;int stateless_rpc =0 ;int i; -char *dir =NULL ; +const char *dir =NULL ;struct command *commands;struct sha1_array shallow =SHA1_ARRAY_INIT ;struct sha1_array ref =SHA1_ARRAY_INIT ;
@@ -1157,7 +1157,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) }if (dir)usage (receive_pack_usage); - dir =xstrdup (arg); + dir = arg; }if (!dir)usage (receive_pack_usage);
Signature · 1 line
2.0.0.566.gfe3e6b2
PATCH 2/5 free ref string returned by dwim_ref
In reply to Jeff KingA call to "dwim_ref(name, len, flags, &ref)" will allocate a new string in "ref" to return the exact ref we found. We do not consistently free it in all code paths, leading to small leaks. The worst is in get_sha1_basic, which may be called many times (e.g., by "cat-file --batch"), though it is relatively unlikely, as it only triggers on a bogus reflog specification.
--- builtin/rev-parse.c | 1 + builtin/show-branch.c | 1 + sha1_name.c | 4 +++- 3 files changed, 5 insertions(+), 1 deletion(-)
builtin/rev-parse.c +1 -0
@@ -151,6 +151,7 @@ static void show_rev(int type, const unsigned char *sha1, const char *name)error ("refname '%s' is ambiguous" , name);break ; } +free (full); }else {show_with_type (type, name); }
builtin/show-branch.c +1 -0
@@ -777,6 +777,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)sprintf (nth_desc,"%s@{%d}" , *av, base+i);append_ref (nth_desc, sha1,1 ); } +free (ref); }else if (all_heads + all_remotes)snarf_refs (all_heads, all_remotes);
sha1_name.c +3 -1
@@ -540,8 +540,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)char *tmp =xstrndup (str + at +2 , reflog_len); at_time =approxidate_careful (tmp, &errors);free (tmp); -if (errors) +if (errors) { +free (real_ref);return -1 ; + } }if (read_ref_at (real_ref, at_time, nth, sha1,NULL , &co_time, &co_tz, &co_cnt)) {
Signature · 1 line
2.0.0.566.gfe3e6b2
PATCH 3/5 transport: fix leaks in refs_from_alternate_cb
In reply to Jeff KingThe function starts by creating a copy of the static buffer returned by real_path, but forgets to free it in the error code paths. We can solve this by jumping to the cleanup code that is already there.
--- transport.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
transport.c +3 -2
@@ -1359,11 +1359,11 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,while (other[len-1 ] =='/' ) other[--len] ='\0' ;if (len <8 ||memcmp (other + len -8 ,"/objects" ,8 )) -return 0 ; + goto out;/* Is this a git repository with refs? */ memcpy (other + len -8 ,"/refs" ,6 );if (!is_directory (other)) -return 0 ; + goto out; other[len -8 ] ='\0' ; remote =remote_get (other); transport =transport_get (remote, other);
@@ -1372,6 +1372,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e, extra = extra->next) cb->fn (extra, cb->data);transport_disconnect (transport); +out:free (other);return 0 ; }
Signature · 1 line
2.0.0.566.gfe3e6b2
PATCH 4/5 fix memory leak parsing core.commentchar
In reply to Jeff KingWhen we see the core.commentchar config option, we extract the string with git_config_string, which does two things:
1. It complains via config_error_nonbool if there is no string value.
2. It makes a copy of the string.
Since we immediately parse the string into its single-character value, we only care about (1). And in fact
(2) is a detriment, as it means we leak the copy. Instead, let's just check the pointer value ourselves, and parse directly from the const string we already have.
--- config.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
config.c +5 -7
@@ -817,14 +817,12 @@ static int git_default_core_config(const char *var, const char *value)return git_config_string (&editor_program, var, value);if (!strcmp (var,"core.commentchar" )) { -const char *comment; -int ret =git_config_string (&comment, var, value); -if (ret) -return ret; -else if (!strcasecmp (comment,"auto" )) +if (!value) +return config_error_nonbool (var); +else if (!strcasecmp (value,"auto" )) auto_comment_line_char =1 ; -else if (comment[0 ] && !comment[1 ]) { - comment_line_char = comment[0 ]; +else if (value[0 ] && !value[1 ]) { + comment_line_char = value[0 ]; auto_comment_line_char =0 ; }else return error ("core.commentChar should only be one character" );
Signature · 1 line
2.0.0.566.gfe3e6b2
PATCH 5/5 apply: avoid possible bogus pointer
In reply to Jeff KingWhen parsing "index" lines from a git-diff, we look for a space followed by the mode. If we don't have a space, then we set our pointer to the end-of-line. However, we don't double-check that our end-of-line pointer is valid (e.g., if we got a truncated diff input), which could lead to some wrap-around pointer arithmetic.
In most cases this would probably get caught by our "40 < len" check later in the function, but to be on the safe side, let's just use strchrnul to treat end-of-string the same as end-of-line.
--- builtin/apply.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/apply.c +1 -1
@@ -1075,7 +1075,7 @@ static int gitdiff_index(const char *line, struct patch *patch) line = ptr +2 ; ptr =strchr (line,' ' ); - eol =strchr (line,'\n' ); + eol =strchrnul (line,'\n' );if (!ptr || eol < ptr) ptr = eol;
Signature · 1 line
2.0.0.566.gfe3e6b2
Quoted 6 lines · Jeff King
Since Stefan has recently started feeding git builds to coverity, I spent a few minutes poking through the results. There are tons of false positives, so there is some work to be done there with tweaking our coverity models. But there are some real issues, too. Here are fixes for the handful that I looked at.
Quoted 7 lines
[1/5]: receive-pack: don't copy "dir" parameter [2/5]: free ref string returned by
dwim_ref[3/5]: transport: fix leaks inrefs_from_alternate_cb[4/5]: fix memory leak parsing core.commentchar [5/5]: apply: avoid possible bogus pointer-Peff
Thanks, they all make sense to me. I'd probably have to wiggle 4/5 a bit to port the fix over to both maint and master, though.
Quoted 6 lines · Jeff King
Since Stefan has recently started feeding git builds to coverity, I spent a few minutes poking through the results. There are tons of false positives, so there is some work to be done there with tweaking our coverity models. But there are some real issues, too. Here are fixes for the handful that I looked at.
Quoted 8 lines
[1/5]: receive-pack: don't copy "dir" parameter [2/5]: free ref string returned by
dwim_ref[3/5]: transport: fix leaks inrefs_from_alternate_cb[4/5]: fix memory leak parsing core.commentchar [5/5]: apply: avoid possible bogus pointer-Peff
As this patchset is in, we have coverity scan now finding 10 defects fixed.
Thanks,
Stefan