diff mbox

[v4] scripts: add sample model file for Coverity Scan

Message ID 1395834349-19001-1-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini March 26, 2014, 11:45 a.m. UTC
This is the model file that is being used for the QEMU project's scans
on scan.coverity.com.  It fixed about 30 false positives (10% of the
total) and exposed about 60 new memory leaks.

The file is not automatically used; changes to it must be propagated
to the website manually by an admin (right now Markus, Peter and me
are admins).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-model.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 183 insertions(+)
 create mode 100644 scripts/coverity-model.c

Comments

Michael Tokarev April 6, 2014, 6:37 a.m. UTC | #1
26.03.2014 15:45, Paolo Bonzini wrote:
> This is the model file that is being used for the QEMU project's scans
> on scan.coverity.com.  It fixed about 30 false positives (10% of the
> total) and exposed about 60 new memory leaks.
> 
> The file is not automatically used; changes to it must be propagated
> to the website manually by an admin (right now Markus, Peter and me
> are admins).

While we don't have issues with the model itself, but really wonder if we
should keep this file inside qemu sources.  It isn't used in there, as
explicitly stated above, and maybe it is easier to maintain it "closer"
to the website?

Thanks,

/mjt
Peter Maydell April 6, 2014, 8:20 a.m. UTC | #2
On 6 April 2014 07:37, Michael Tokarev <mjt@tls.msk.ru> wrote:
> While we don't have issues with the model itself, but really wonder if we
> should keep this file inside qemu sources.  It isn't used in there, as
> explicitly stated above, and maybe it is easier to maintain it "closer"
> to the website?

Where would that be? There's no version control there. The
other alternative would be somebody's personal git repo,
which seems a recipe for it being impossible to find.

thanks
-- PMM
Stefan Weil April 6, 2014, 8:35 a.m. UTC | #3
Am 06.04.2014 10:20, schrieb Peter Maydell:
> On 6 April 2014 07:37, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> While we don't have issues with the model itself, but really wonder if we
>> should keep this file inside qemu sources.  It isn't used in there, as
>> explicitly stated above, and maybe it is easier to maintain it "closer"
>> to the website?
> 
> Where would that be? There's no version control there. The
> other alternative would be somebody's personal git repo,
> which seems a recipe for it being impossible to find.
> 
> thanks
> -- PMM
> 

I suggest documenting the use of coverity in the QEMU wiki.
http://wiki.qemu.org/Testing is a good place for that.

The model file could also be placed there on a new page: mediawiki has
version control, and maintenance would be easier than with patches sent
to the mailing list. People who want to review any changes just have to
get a Wiki account (most already have, I assume) and watch the page for
modifications.

Regards
Stefan
Stefan Weil April 6, 2014, 8:41 a.m. UTC | #4
Am 06.04.2014 10:35, schrieb Stefan Weil:
> Am 06.04.2014 10:20, schrieb Peter Maydell:
>> On 6 April 2014 07:37, Michael Tokarev <mjt@tls.msk.ru> wrote:
>>> While we don't have issues with the model itself, but really wonder if we
>>> should keep this file inside qemu sources.  It isn't used in there, as
>>> explicitly stated above, and maybe it is easier to maintain it "closer"
>>> to the website?
>>
>> Where would that be? There's no version control there. The
>> other alternative would be somebody's personal git repo,
>> which seems a recipe for it being impossible to find.
>>
>> thanks
>> -- PMM
>>
> 
> I suggest documenting the use of coverity in the QEMU wiki.
> http://wiki.qemu.org/Testing is a good place for that.
> 
> The model file could also be placed there on a new page: mediawiki has
> version control, and maintenance would be easier than with patches sent
> to the mailing list. People who want to review any changes just have to
> get a Wiki account (most already have, I assume) and watch the page for
> modifications.
> 
> Regards
> Stefan

I forgot to mention that a license change would be needed for the model
file because the Wiki uses the GNU Free Documentation License 1.2.

Stefan
Dr. David Alan Gilbert April 7, 2014, 8:10 a.m. UTC | #5
* Michael Tokarev (mjt@tls.msk.ru) wrote:
> 26.03.2014 15:45, Paolo Bonzini wrote:
> > This is the model file that is being used for the QEMU project's scans
> > on scan.coverity.com.  It fixed about 30 false positives (10% of the
> > total) and exposed about 60 new memory leaks.
> > 
> > The file is not automatically used; changes to it must be propagated
> > to the website manually by an admin (right now Markus, Peter and me
> > are admins).
> 
> While we don't have issues with the model itself, but really wonder if we
> should keep this file inside qemu sources.  It isn't used in there, as
> explicitly stated above, and maybe it is easier to maintain it "closer"
> to the website?

Given that the model file describes semantics of functions in the codebase
it needs to be kept consistent with the version of the code.
Hence it needs to be version controlled with the code, hence the best place
is inside the qemu sources.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Michael Tokarev April 7, 2014, 9:17 a.m. UTC | #6
26.03.2014 15:45, Paolo Bonzini wrote:
> This is the model file that is being used for the QEMU project's scans
> on scan.coverity.com.  It fixed about 30 false positives (10% of the
> total) and exposed about 60 new memory leaks.
> 
> The file is not automatically used; changes to it must be propagated
> to the website manually by an admin (right now Markus, Peter and me
> are admins).

Applied to -trivial, thank you!

Note: I haven't actually checked this file for correctness myself ;)

/mjt
Markus Armbruster April 7, 2014, 9:58 a.m. UTC | #7
Michael Tokarev <mjt@tls.msk.ru> writes:

> 26.03.2014 15:45, Paolo Bonzini wrote:
>> This is the model file that is being used for the QEMU project's scans
>> on scan.coverity.com.  It fixed about 30 false positives (10% of the
>> total) and exposed about 60 new memory leaks.
>> 
>> The file is not automatically used; changes to it must be propagated
>> to the website manually by an admin (right now Markus, Peter and me
>> are admins).
>
> Applied to -trivial, thank you!
>
> Note: I haven't actually checked this file for correctness myself ;)

I checked v2.  I suggested a few minor improvements, and some of them
made it into this version.

Since Paolo's initial version comes in part from me, I guess it would be
proper to add

Signed-off-by: Markus Armbruster <armbru@redhat.com>
diff mbox

Patch

diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
new file mode 100644
index 0000000..4c99a85
--- /dev/null
+++ b/scripts/coverity-model.c
@@ -0,0 +1,183 @@ 
+/* Coverity Scan model
+ *
+ * Copyright (C) 2014 Red Hat, Inc.
+ *
+ * Authors:
+ *  Markus Armbruster <armbru@redhat.com>
+ *  Paolo Bonzini <pbonzini@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or, at your
+ * option, any later version.  See the COPYING file in the top-level directory.
+ */
+
+
+/*
+ * This is the source code for our Coverity user model file.  The
+ * purpose of user models is to increase scanning accuracy by explaining
+ * code Coverity can't see (out of tree libraries) or doesn't
+ * sufficiently understand.  Better accuracy means both fewer false
+ * positives and more true defects.  Memory leaks in particular.
+ *
+ * - A model file can't import any header files.  Some built-in primitives are
+ *   available but not wchar_t, NULL etc.
+ * - Modeling doesn't need full structs and typedefs. Rudimentary structs
+ *   and similar types are sufficient.
+ * - An uninitialized local variable signifies that the variable could be
+ *   any value.
+ *
+ * The model file must be uploaded by an admin in the analysis settings of
+ * http://scan.coverity.com/projects/378
+ */
+
+#define NULL ((void *)0)
+
+typedef unsigned char uint8_t;
+typedef char int8_t;
+typedef unsigned int uint32_t;
+typedef int int32_t;
+typedef long ssize_t;
+typedef unsigned long long uint64_t;
+typedef long long int64_t;
+typedef _Bool bool;
+
+/* exec.c */
+
+typedef struct AddressSpace AddressSpace;
+typedef uint64_t hwaddr;
+
+static void __write(uint8_t *buf, ssize_t len)
+{
+    int first, last;
+    __coverity_negative_sink__(len);
+    if (len == 0) return;
+    buf[0] = first;
+    buf[len-1] = last;
+    __coverity_writeall__(buf);
+}
+
+static void __read(uint8_t *buf, ssize_t len)
+{
+    __coverity_negative_sink__(len);
+    if (len == 0) return;
+    int first = buf[0];
+    int last = buf[len-1];
+}
+
+bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
+                      int len, bool is_write)
+{
+    bool result;
+
+    // TODO: investigate impact of treating reads as producing
+    // tainted data, with __coverity_tainted_data_argument__(buf).
+    if (is_write) __write(buf, len); else __read(buf, len);
+
+    return result;
+}
+
+/* Tainting */
+
+typedef struct {} name2keysym_t;
+static int get_keysym(const name2keysym_t *table,
+                      const char *name)
+{
+    int result;
+    if (result > 0) {
+        __coverity_tainted_string_sanitize_content__(name);
+        return result;
+    } else {
+        return 0;
+    }
+}
+
+/* glib memory allocation functions.
+ *
+ * Note that we ignore the fact that g_malloc of 0 bytes returns NULL,
+ * and g_realloc of 0 bytes frees the pointer.
+ *
+ * Modeling this would result in Coverity flagging a lot of memory
+ * allocations as potentially returning NULL, and asking us to check
+ * whether the result of the allocation is NULL or not.  However, the
+ * resulting pointer should never be dereferenced anyway, and in fact
+ * it is not in the vast majority of cases.
+ *
+ * If a dereference did happen, this would suppress a defect report
+ * for an actual null pointer dereference.  But it's too unlikely to
+ * be worth wading through the false positives, and with some luck
+ * we'll get a buffer overflow reported anyway.
+ */
+
+void *malloc(size_t);
+void *calloc(size_t, size_t);
+void *realloc(void *, size_t);
+void free(void *);
+
+void *
+g_malloc(size_t n_bytes)
+{
+    void *mem;
+    __coverity_negative_sink__(n_bytes);
+    mem = malloc(n_bytes == 0 ? 1 : n_bytes);
+    if (!mem) __coverity_panic__();
+    return mem;
+}
+
+void *
+g_malloc0(size_t n_bytes)
+{
+    void *mem;
+    __coverity_negative_sink__(n_bytes);
+    mem = calloc(1, n_bytes == 0 ? 1 : n_bytes);
+    if (!mem) __coverity_panic__();
+    return mem;
+}
+
+void g_free(void *mem)
+{
+    free(mem);
+}
+
+void *g_realloc(void * mem, size_t n_bytes)
+{
+    __coverity_negative_sink__(n_bytes);
+    mem = realloc(mem, n_bytes == 0 ? 1 : n_bytes);
+    if (!mem) __coverity_panic__();
+    return mem;
+}
+
+void *g_try_malloc(size_t n_bytes)
+{
+    __coverity_negative_sink__(n_bytes);
+    return malloc(n_bytes == 0 ? 1 : n_bytes);
+}
+
+void *g_try_malloc0(size_t n_bytes)
+{
+    __coverity_negative_sink__(n_bytes);
+    return calloc(1, n_bytes == 0 ? 1 : n_bytes);
+}
+
+void *g_try_realloc(void *mem, size_t n_bytes)
+{
+    __coverity_negative_sink__(n_bytes);
+    return realloc(mem, n_bytes == 0 ? 1 : n_bytes);
+}
+
+/* Other glib functions */
+
+typedef struct _GIOChannel GIOChannel;
+GIOChannel *g_io_channel_unix_new(int fd)
+{
+    GIOChannel *c = g_malloc0(sizeof(GIOChannel));
+    __coverity_escape__(fd);
+    return c;
+}
+
+void g_assertion_message_expr(const char     *domain,
+                              const char     *file,
+                              int             line,
+                              const char     *func,
+                              const char     *expr)
+{
+    __coverity_panic__();
+}