[Qemu-devel] [PATCH 0/3] coverity: Improve and extend model

[Qemu-devel] [PATCH 0/3] coverity: Improve and extend model

[Markus Armbruster]

I examined the differences between local scans with and without a
derived model file for GLib, to gauge what we're missing (the Coverity
Scan service we use can't do derived model files).  Doesn't look bad,
but a few missed memory leaks caught my attention.  I improved our
model file to catch them (PATCH 1+2).  Topped off with PATCH 3 to
catch mixing up g_free() and free().

Markus Armbruster (3):
  coverity: Improve model for GLib memory allocation
  coverity: Model GLib string allocation partially
  coverity: Model g_free() isn't necessarily free()

 scripts/coverity-model.c | 228 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 193 insertions(+), 35 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/3] coverity: Improve model for GLib memory allocation

[Markus Armbruster]

In current versions of GLib, g_new() may expand into g_malloc_n().
When it does, Coverity can't see the memory allocation, because we
don't model g_malloc_n().  Similarly for g_new0(), g_renew(),
g_try_new(), g_try_new0(), g_try_renew().

Model g_malloc_n(), g_malloc0_n(), g_realloc_n().  Model
g_try_malloc_n(), g_try_malloc0_n(), g_try_realloc_n() by adding
indeterminate out of memory conditions on top.

To avoid undue duplication, replace the existing models for g_malloc()
& friends by trivial wrappers around g_malloc_n() & friends.

In a local scan, this flags four additional RESOURCE_LEAKs and one
NULL_RETURNS.

The NULL_RETURNS is a false positive: Coverity can now see that
g_try_malloc(l1_sz * sizeof(uint64_t)) in
qcow2_check_metadata_overlap() may return NULL, but is too stupid to
recognize that a loop executing l1_sz times won't be entered then.

Three out of the four RESOURCE_LEAKs appear genuine.  The false
positive is in ppce500_prep_device_tree(): the pointer dies, but a
pointer to a struct member escapes, and we get the pointer back for
freeing with container_of().  Too funky for Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/coverity-model.c | 139 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 104 insertions(+), 35 deletions(-)

diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
index 4c99a85..8d0839e 100644
--- a/scripts/coverity-model.c
+++ b/scripts/coverity-model.c
@@ -90,7 +90,8 @@ static int get_keysym(const name2keysym_t *table,
     }
 }
 
-/* glib memory allocation functions.
+/*
+ * 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.
@@ -107,60 +108,128 @@ static int get_keysym(const name2keysym_t *table,
  * 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 *);
+/*
+ * Allocation primitives, cannot return NULL
+ * See also Coverity's library/generic/libc/all/all.c
+ */
 
-void *
-g_malloc(size_t n_bytes)
+void *g_malloc_n(size_t nmemb, size_t size)
 {
-    void *mem;
-    __coverity_negative_sink__(n_bytes);
-    mem = malloc(n_bytes == 0 ? 1 : n_bytes);
-    if (!mem) __coverity_panic__();
-    return mem;
+    size_t sz;
+    void *ptr;
+
+    __coverity_negative_sink__(nmemb);
+    __coverity_negative_sink__(size);
+    sz = nmemb * size;
+    ptr = __coverity_alloc__(size);
+    __coverity_mark_as_uninitialized_buffer__(ptr);
+    __coverity_mark_as_afm_allocated__(ptr, AFM_free);
+    return ptr;
+}
+
+void *g_malloc0_n(size_t nmemb, size_t size)
+{
+    size_t sz;
+    void *ptr;
+
+    __coverity_negative_sink__(nmemb);
+    __coverity_negative_sink__(size);
+    sz = nmemb * size;
+    ptr = __coverity_alloc__(size);
+    __coverity_writeall0__(ptr);
+    __coverity_mark_as_afm_allocated__(ptr, AFM_free);
+    return ptr;
+}
+
+void *g_realloc_n(void *ptr, size_t nmemb, size_t size)
+{
+    size_t sz;
+
+    __coverity_negative_sink__(nmemb);
+    __coverity_negative_sink__(size);
+    sz = nmemb * size;
+    __coverity_escape__(ptr);
+    ptr = __coverity_alloc__(size);
+    /*
+     * Memory beyond the old size isn't actually initialized.  Can't
+     * model that.  See Coverity's realloc() model
+     */
+    __coverity_writeall__(ptr);
+    __coverity_mark_as_afm_allocated__(ptr, AFM_free);
+    return ptr;
+}
+
+void g_free(void *ptr)
+{
+    __coverity_free__(ptr);
+    __coverity_mark_as_afm_freed__(ptr, AFM_free);
+}
+
+/*
+ * Derive the g_try_FOO_n() from the g_FOO_n() by adding indeterminate
+ * out of memory conditions
+ */
+
+void *g_try_malloc_n(size_t nmemb, size_t size)
+{
+    int nomem;
+
+    if (nomem) {
+        return NULL;
+    }
+    return g_malloc_n(nmemb, size);
 }
 
-void *
-g_malloc0(size_t n_bytes)
+void *g_try_malloc0_n(size_t nmemb, size_t size)
+{
+    int nomem;
+
+    if (nomem) {
+        return NULL;
+    }
+    return g_malloc0_n(nmemb, size);
+}
+
+void *g_try_realloc_n(void *ptr, size_t nmemb, size_t size)
+{
+    int nomem;
+
+    if (nomem) {
+        return NULL;
+    }
+    return g_realloc_n(ptr, nmemb, size);
+}
+
+/* Trivially derive the g_FOO() from the g_FOO_n() */
+
+void *g_malloc(size_t size)
 {
-    void *mem;
-    __coverity_negative_sink__(n_bytes);
-    mem = calloc(1, n_bytes == 0 ? 1 : n_bytes);
-    if (!mem) __coverity_panic__();
-    return mem;
+    return g_malloc_n(1, size);
 }
 
-void g_free(void *mem)
+void *g_malloc0(size_t size)
 {
-    free(mem);
+    return g_malloc0_n(1, size);
 }
 
-void *g_realloc(void * mem, size_t n_bytes)
+void *g_realloc(void *ptr, size_t size)
 {
-    __coverity_negative_sink__(n_bytes);
-    mem = realloc(mem, n_bytes == 0 ? 1 : n_bytes);
-    if (!mem) __coverity_panic__();
-    return mem;
+    return g_realloc_n(ptr, 1, size);
 }
 
-void *g_try_malloc(size_t n_bytes)
+void *g_try_malloc(size_t size)
 {
-    __coverity_negative_sink__(n_bytes);
-    return malloc(n_bytes == 0 ? 1 : n_bytes);
+    return g_try_malloc_n(1, size);
 }
 
-void *g_try_malloc0(size_t n_bytes)
+void *g_try_malloc0(size_t size)
 {
-    __coverity_negative_sink__(n_bytes);
-    return calloc(1, n_bytes == 0 ? 1 : n_bytes);
+    return g_try_malloc0_n(1, size);
 }
 
-void *g_try_realloc(void *mem, size_t n_bytes)
+void *g_try_realloc(void *ptr, size_t size)
 {
-    __coverity_negative_sink__(n_bytes);
-    return realloc(mem, n_bytes == 0 ? 1 : n_bytes);
+    return g_try_realloc_n(ptr, 1, size);
 }
 
 /* Other glib functions */
-- 
1.9.3

[Qemu-devel] [PATCH 2/3] coverity: Model GLib string allocation partially

[Markus Armbruster]

Without a model, Coverity can't know that the result of g_strdup()
needs to be fed to g_free().

One way to get such a model is to scan GLib, build a derived model
file with cov-collect-models, and use that when scanning QEMU.
Unfortunately, the Coverity Scan service we use doesn't support that.

Thus, we're stuck with the other way: write a user model.  Doing that
for all of GLib is hardly practical.  I'm doing it for the "String
Utility Functions" we actually use that return dynamically allocated
strings.

In a local scan, this flags 20 additional RESOURCE_LEAKs.  The ones I
checked look genuine.

It also loses a NULL_RETURNS about ppce500_init() using
qemu_find_file() without error checking.  I don't understand why.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/coverity-model.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
index 8d0839e..230bc30 100644
--- a/scripts/coverity-model.c
+++ b/scripts/coverity-model.c
@@ -40,6 +40,8 @@ typedef unsigned long long uint64_t;
 typedef long long int64_t;
 typedef _Bool bool;
 
+typedef struct va_list_str *va_list;
+
 /* exec.c */
 
 typedef struct AddressSpace AddressSpace;
@@ -232,6 +234,93 @@ void *g_try_realloc(void *ptr, size_t size)
     return g_try_realloc_n(ptr, 1, size);
 }
 
+/*
+ * GLib string allocation functions
+ */
+
+char *g_strdup(const char *s)
+{
+    char *dup;
+    size_t i;
+
+    if (!s) {
+        return NULL;
+    }
+
+    __coverity_string_null_sink__(s);
+    __coverity_string_size_sink__(s);
+    dup = __coverity_alloc_nosize__();
+    __coverity_mark_as_afm_allocated__(dup, AFM_free);
+    for (i = 0; (dup[i] = s[i]); i++) ;
+    return dup;
+}
+
+char *g_strndup(const char *s, size_t n)
+{
+    char *dup;
+    size_t i;
+
+    __coverity_negative_sink__(n);
+
+    if (!s) {
+        return NULL;
+    }
+
+    dup = g_malloc(n + 1);
+    for (i = 0; i < n && (dup[i] = s[i]); i++) ;
+    dup[i] = 0;
+    return dup;
+}
+
+char *g_strdup_printf(const char *format, ...)
+{
+    char ch, *s;
+    size_t len;
+
+    __coverity_string_null_sink__(format);
+    __coverity_string_size_sink__(format);
+
+    ch = *format;
+
+    s = __coverity_alloc_nosize__();
+    __coverity_writeall__(s);
+    __coverity_mark_as_afm_allocated__(s, AFM_free);
+    return s;
+}
+
+char *g_strdup_vprintf(const char *format, va_list ap)
+{
+    char ch, *s;
+    size_t len;
+
+    __coverity_string_null_sink__(format);
+    __coverity_string_size_sink__(format);
+
+    ch = *format;
+    ch = *(char *)ap;
+
+    s = __coverity_alloc_nosize__();
+    __coverity_writeall__(s);
+    __coverity_mark_as_afm_allocated__(s, AFM_free);
+
+    return len;
+}
+
+char *g_strconcat(const char *s, ...)
+{
+    char *s;
+
+    /*
+     * Can't model: last argument must be null, the others
+     * null-terminated strings
+     */
+
+    s = __coverity_alloc_nosize__();
+    __coverity_writeall__(s);
+    __coverity_mark_as_afm_allocated__(s, AFM_free);
+    return s;
+}
+
 /* Other glib functions */
 
 typedef struct _GIOChannel GIOChannel;
-- 
1.9.3

[Qemu-devel] [PATCH 3/3] coverity: Model g_free() isn't necessarily free()

[Markus Armbruster]

Memory allocated with GLib needs to be freed with GLib.  Freeing it
with free() instead of g_free() is a common error.  Harmless when
g_free() is a trivial wrapper around free(), which is commonly the
case.  But model the difference anyway.

In a local scan, this flags four ALLOC_FREE_MISMATCH.  Requires
--enable ALLOC_FREE_MISMATCH, because the checker is still preview.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/coverity-model.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
index 230bc30..58356af 100644
--- a/scripts/coverity-model.c
+++ b/scripts/coverity-model.c
@@ -125,7 +125,7 @@ void *g_malloc_n(size_t nmemb, size_t size)
     sz = nmemb * size;
     ptr = __coverity_alloc__(size);
     __coverity_mark_as_uninitialized_buffer__(ptr);
-    __coverity_mark_as_afm_allocated__(ptr, AFM_free);
+    __coverity_mark_as_afm_allocated__(ptr, "g_free");
     return ptr;
 }
 
@@ -139,7 +139,7 @@ void *g_malloc0_n(size_t nmemb, size_t size)
     sz = nmemb * size;
     ptr = __coverity_alloc__(size);
     __coverity_writeall0__(ptr);
-    __coverity_mark_as_afm_allocated__(ptr, AFM_free);
+    __coverity_mark_as_afm_allocated__(ptr, "g_free");
     return ptr;
 }
 
@@ -157,14 +157,14 @@ void *g_realloc_n(void *ptr, size_t nmemb, size_t size)
      * model that.  See Coverity's realloc() model
      */
     __coverity_writeall__(ptr);
-    __coverity_mark_as_afm_allocated__(ptr, AFM_free);
+    __coverity_mark_as_afm_allocated__(ptr, "g_free");
     return ptr;
 }
 
 void g_free(void *ptr)
 {
     __coverity_free__(ptr);
-    __coverity_mark_as_afm_freed__(ptr, AFM_free);
+    __coverity_mark_as_afm_freed__(ptr, "g_free");
 }
 
 /*
@@ -250,7 +250,7 @@ char *g_strdup(const char *s)
     __coverity_string_null_sink__(s);
     __coverity_string_size_sink__(s);
     dup = __coverity_alloc_nosize__();
-    __coverity_mark_as_afm_allocated__(dup, AFM_free);
+    __coverity_mark_as_afm_allocated__(dup, "g_free");
     for (i = 0; (dup[i] = s[i]); i++) ;
     return dup;
 }
@@ -284,7 +284,7 @@ char *g_strdup_printf(const char *format, ...)
 
     s = __coverity_alloc_nosize__();
     __coverity_writeall__(s);
-    __coverity_mark_as_afm_allocated__(s, AFM_free);
+    __coverity_mark_as_afm_allocated__(s, "g_free");
     return s;
 }
 
@@ -301,7 +301,7 @@ char *g_strdup_vprintf(const char *format, va_list ap)
 
     s = __coverity_alloc_nosize__();
     __coverity_writeall__(s);
-    __coverity_mark_as_afm_allocated__(s, AFM_free);
+    __coverity_mark_as_afm_allocated__(s, "g_free");
 
     return len;
 }
@@ -317,7 +317,7 @@ char *g_strconcat(const char *s, ...)
 
     s = __coverity_alloc_nosize__();
     __coverity_writeall__(s);
-    __coverity_mark_as_afm_allocated__(s, AFM_free);
+    __coverity_mark_as_afm_allocated__(s, "g_free");
     return s;
 }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 0/3] coverity: Improve and extend model

[Paolo Bonzini]

On 28/01/2015 10:58, Markus Armbruster wrote:
> I examined the differences between local scans with and without a
> derived model file for GLib, to gauge what we're missing (the Coverity
> Scan service we use can't do derived model files).  Doesn't look bad,
> but a few missed memory leaks caught my attention.  I improved our
> model file to catch them (PATCH 1+2).  Topped off with PATCH 3 to
> catch mixing up g_free() and free().
> 
> Markus Armbruster (3):
>   coverity: Improve model for GLib memory allocation
>   coverity: Model GLib string allocation partially
>   coverity: Model g_free() isn't necessarily free()
> 
>  scripts/coverity-model.c | 228 +++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 193 insertions(+), 35 deletions(-)
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

It's missing a patch to add a MAINTAINERS entry though! :)

Paolo

Re: [Qemu-devel] [PATCH 0/3] coverity: Improve and extend model

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 28/01/2015 10:58, Markus Armbruster wrote:
>> I examined the differences between local scans with and without a
>> derived model file for GLib, to gauge what we're missing (the Coverity
>> Scan service we use can't do derived model files).  Doesn't look bad,
>> but a few missed memory leaks caught my attention.  I improved our
>> model file to catch them (PATCH 1+2).  Topped off with PATCH 3 to
>> catch mixing up g_free() and free().
>> 
>> Markus Armbruster (3):
>>   coverity: Improve model for GLib memory allocation
>>   coverity: Model GLib string allocation partially
>>   coverity: Model g_free() isn't necessarily free()
>> 
>>  scripts/coverity-model.c | 228 +++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 193 insertions(+), 35 deletions(-)
>> 
>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks!

> It's missing a patch to add a MAINTAINERS entry though! :)

Might as well.

[Qemu-devel] [PATCH 4/3] MAINTAINERS: Add myself as Coverity model maintainer

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 MAINTAINERS | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 430688d..b1802b0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -778,6 +778,11 @@ M: Samuel Thibault <samuel.thibault@ens-lyon.org>
 S: Maintained
 F: backends/baum.c
 
+Coverity model
+M: Markus Armbruster <armbru@redhat.com>
+S: Supported
+F: scripts/coverity-model.c
+
 CPU
 M: Andreas Färber <afaerber@suse.de>
 S: Supported
-- 
1.9.3