[PATCH 0/3] Fix some issues found by Coverity

[PATCH 0/3] Fix some issues found by Coverity

[john]

These fix the three issues found by Coverity that are definitely
incorrect.  There are seven more issues which mostly look like false
positives but I'm not sure if we should simply mark them as such or try
to improve the situation with modelling.

John Keeping (3):
  ui-log: handle parse_commit() errors
  cache: use size_t for string lengths
  cache: don't check for match with no key

 cache.c  | 9 +++++----
 ui-log.c | 4 +++-
 2 files changed, 8 insertions(+), 5 deletions(-)

-- 
2.7.0.226.gfe986fe

[PATCH 1/3] ui-log: handle parse_commit() errors

[john]

If parse_commit() fails, none of the fields in the commit structure will
have been populated so we will dereference NULL when accessing
item->tree.

There isn't much we can do about the error at this point, but if we
return true then we'll try parsing the commit again from print_commit()
and we can report an error to the user at that point.

Coverity-id: 13801
Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-log.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ui-log.c b/ui-log.c
index 4573255..a4dc707 100644
--- a/ui-log.c
+++ b/ui-log.c
@@ -141,7 +141,9 @@ static int show_commit(struct commit *commit, struct rev_info *revs)
 
 	/* When we get here we have precisely one parent. */
 	parent = parents->item;
-	parse_commit(parent);
+	/* If we can't parse the commit, let print_commit() report an error. */
+	if (parse_commit(parent))
+		return 1;
 
 	files = 0;
 	add_lines = 0;
-- 
2.7.0.226.gfe986fe

[PATCH 2/3] cache: use size_t for string lengths

[john]

Avoid integer truncation on 64-bit systems.

Coverity-id: 13864
Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cache.c b/cache.c
index b169d20..df1b4a3 100644
--- a/cache.c
+++ b/cache.c
@@ -24,7 +24,7 @@
 
 struct cache_slot {
 	const char *key;
-	int keylen;
+	size_t keylen;
 	int ttl;
 	cache_fill_fn fn;
 	int cache_fd;
@@ -44,7 +44,7 @@ struct cache_slot {
 static int open_slot(struct cache_slot *slot)
 {
 	char *bufz;
-	int bufkeylen = -1;
+	ssize_t bufkeylen = -1;
 
 	slot->cache_fd = open(slot->cache_name, O_RDONLY);
 	if (slot->cache_fd == -1)
-- 
2.7.0.226.gfe986fe

[PATCH 3/3] cache: don't check for match with no key

[john]

We call open_slot() from cache_ls() without a key since we simply want
to read the path out of the header.  Should the file happen to contain
an empty key then we end up calling memcmp() with NULL and a non-zero
length.  Fix this by assigning slot->match only if a key is set, which
is always will be in the code paths where we use slot->match.

Coverity-id: 13807
Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cache.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/cache.c b/cache.c
index df1b4a3..6736a01 100644
--- a/cache.c
+++ b/cache.c
@@ -61,8 +61,9 @@ static int open_slot(struct cache_slot *slot)
 	if (bufz)
 		bufkeylen = bufz - slot->buf;
 
-	slot->match = bufkeylen == slot->keylen &&
-	    !memcmp(slot->key, slot->buf, bufkeylen + 1);
+	if (slot->key)
+		slot->match = bufkeylen == slot->keylen &&
+		    !memcmp(slot->key, slot->buf, bufkeylen + 1);
 
 	return 0;
 }
-- 
2.7.0.226.gfe986fe

[PATCH 0/3] Fix some issues found by Coverity

[Jason]

Thanks John!

[PATCH 0/3] Fix some issues found by Coverity

[Jason]

So there is now only 1 issue remaining: 13839.

static void add_commit(struct string_list *authors, struct commit *commit,
        const struct cgit_period *period)
{
        struct commitinfo *info;
        struct string_list_item *author, *item;
        struct authorstat *authorstat;
        struct string_list *items;
        char *tmp;
        struct tm *date;
        time_t t;

        info = cgit_parse_commit(commit);
        tmp = xstrdup(info->author);
        author = string_list_insert(authors, tmp);
        if (!author->util)
                author->util = xcalloc(1, sizeof(struct authorstat));
        else
                free(tmp);
        authorstat = author->util;
        items = &authorstat->list;
        t = info->committer_date;
        date = gmtime(&t);
        period->trunc(date);
        tmp = xstrdup(period->pretty(date));
        item = string_list_insert(items, tmp);
        if (item->util)
                free(tmp);
        item->util++;
        authorstat->total++;
        cgit_free_commitinfo(info);
}

It doesn't like the "item->util++" line, since "if (item->util)"
implies that util could be NULL. That line doesn't make much sense to
me either. Any guesses?


Jason

[PATCH 0/3] Fix some issues found by Coverity

[john]

On Sun, Jan 17, 2016 at 05:19:04PM +0100, Jason A. Donenfeld wrote:
> So there is now only 1 issue remaining: 13839.
> 
> static void add_commit(struct string_list *authors, struct commit *commit,
>         const struct cgit_period *period)
> {
>         struct commitinfo *info;
>         struct string_list_item *author, *item;
>         struct authorstat *authorstat;
>         struct string_list *items;
>         char *tmp;
>         struct tm *date;
>         time_t t;
> 
>         info = cgit_parse_commit(commit);
>         tmp = xstrdup(info->author);
>         author = string_list_insert(authors, tmp);
>         if (!author->util)
>                 author->util = xcalloc(1, sizeof(struct authorstat));
>         else
>                 free(tmp);
>         authorstat = author->util;
>         items = &authorstat->list;
>         t = info->committer_date;
>         date = gmtime(&t);
>         period->trunc(date);
>         tmp = xstrdup(period->pretty(date));
>         item = string_list_insert(items, tmp);
>         if (item->util)
>                 free(tmp);
>         item->util++;
>         authorstat->total++;
>         cgit_free_commitinfo(info);
> }
> 
> It doesn't like the "item->util++" line, since "if (item->util)"
> implies that util could be NULL. That line doesn't make much sense to
> me either. Any guesses?

We're using "util" as a counter here, not a pointer.  But it's declared
as "void*" so Coverity doesn't like this.

We could try adding in some casts to uintptr_t but that's pretty
hideous.  Otherwise we need to allocate an unsigned int for the "util"
field or just ignore Coverity.

[PATCH 0/3] Fix some issues found by Coverity

[mathstuf]

On Sun, 17 Jan, 2016 at 20:25:36 GMT, John Keeping wrote:
> We're using "util" as a counter here, not a pointer.  But it's declared
> as "void*" so Coverity doesn't like this.
>
> We could try adding in some casts to uintptr_t but that's pretty
> hideous.  Otherwise we need to allocate an unsigned int for the "util"
> field or just ignore Coverity.

Incrementing a void* is undefined behavior (what is the step size?).

    https://stackoverflow.com/questions/6449935/increment-void-pointer-by-one-byte-by-two

--Ben

[PATCH 0/3] Fix some issues found by Coverity

[Jason]

http://git.zx2c4.com/cgit/commit/?id=a8b9ef8c1c68fbb9c89db2d8c12dca38c15e2bfd

[PATCH 0/3] Fix some issues found by Coverity

[john]

On Mon, Feb 08, 2016 at 02:35:10PM +0100, Jason A. Donenfeld wrote:
> http://git.zx2c4.com/cgit/commit/?id=a8b9ef8c1c68fbb9c89db2d8c12dca38c15e2bfd

I'm not sure that quite goes far enough.  Do you want to add this on
top?

-- >8 --
From fad329930c9f57e0d75d9094bc70532283bd2b79 Mon Sep 17 00:00:00 2001
From: John Keeping <john at keeping.me.uk>
Date: Mon, 8 Feb 2016 14:12:35 +0000
Subject: [PATCH] ui-stats: cast pointer before checking for zero

We abuse the "void *util" field as a counter and recently started to
cast it to a uintptr_t to avoid risking nasal demons by performing
arithmetic on a void pointer.

However, compilers are also known to do "interesting" things if they
know that a pointer is or isn't NULL.  Make this safer by checking if
the counter (after casting) is non-zero rather than checking if the
pointer is non-null.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-stats.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui-stats.c b/ui-stats.c
index a9c13fd..8cd9178 100644
--- a/ui-stats.c
+++ b/ui-stats.c
@@ -184,9 +184,9 @@ static void add_commit(struct string_list *authors, struct commit *commit,
 	period->trunc(date);
 	tmp = xstrdup(period->pretty(date));
 	item = string_list_insert(items, tmp);
-	if (item->util)
-		free(tmp);
 	counter = (uintptr_t *)&item->util;
+	if (*counter)
+		free(tmp);
 	(*counter)++;
 
 	authorstat->total++;
-- 
2.7.0.389.ga73dcac

[PATCH 0/3] Fix some issues found by Coverity

[Jason]

Merged, thanks.

[PATCH 0/3] Fix some issues found by Coverity

[john]

On Mon, Feb 08, 2016 at 03:33:07PM +0100, Jason A. Donenfeld wrote:
> Merged, thanks.

Did you mean to merge this to master as well as jd/zx2c4-deployment?
(I just want to make sure it doesn't get lost in a future rebase, not a
problem if this was intentional to test it before applying to master.)

[PATCH 0/3] Fix some issues found by Coverity

[Jason]

Whoops. Thanks.