[Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan

[Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan

[Paolo Bonzini]

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 | 170 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 170 insertions(+)
 create mode 100644 scripts/coverity-model.c

diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
new file mode 100644
index 0000000..1cc0c1b
--- /dev/null
+++ b/scripts/coverity-model.c
@@ -0,0 +1,170 @@
+/* Coverity Scan model
+ *
+ * Copyright (C) 2014 Red Hat, Inc.
+ *
+ * Authors:
+ *  Markus Armbruster <armbru@redhat.com>
+ *  Paolo Bonzini <pbonzini@redhat.com>
+ *
+ * This is a modeling file for Coverity Scan. Modeling helps to avoid false
+ * positives.
+ *
+ * - 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
+#define assert(x) if (!(x)) __coverity_panic__();
+
+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, int 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, int 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;
+
+    // perhaps __coverity_tainted_data_argument__(buf); for read,
+    // and __coverity_tainted_data_sink__(buf); for write?
+    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 */
+
+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__((ssize_t) 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__((ssize_t) n_bytes);
+    mem = calloc(1, n_bytes == 0 ? 1 : n_bytes);
+    if (!mem) __coverity_panic__ ();
+    return mem;
+}
+
+void g_free (void *mem)
+{
+    if (mem) {
+        free(mem);
+    }
+}
+
+void *g_realloc (void * mem, size_t n_bytes)
+{
+    __coverity_negative_sink__((ssize_t) n_bytes);
+    if (n_bytes == 0) {
+        g_free(mem);
+        return NULL;
+    } else {
+        mem = realloc(mem, n_bytes);
+        if (!mem) __coverity_panic__ ();
+        return mem;
+    }
+}
+
+void *g_try_malloc (size_t n_bytes)
+{
+    __coverity_negative_sink__((ssize_t) n_bytes);
+    return malloc(n_bytes == 0 ? 1 : n_bytes);
+}
+
+void *g_try_malloc0 (size_t n_bytes)
+{
+    __coverity_negative_sink__((ssize_t) n_bytes);
+    return calloc(1, n_bytes == 0 ? 1 : n_bytes);
+}
+
+void *g_try_realloc (void *mem, size_t n_bytes)
+{
+    __coverity_negative_sink__((ssize_t) n_bytes);
+    if (n_bytes == 0) {
+        g_free(mem);
+        return NULL;
+    } else {
+        return realloc(mem, n_bytes);
+    }
+}
+
+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__();
+}
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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 | 170 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 170 insertions(+)
>  create mode 100644 scripts/coverity-model.c
>
> diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
> new file mode 100644
> index 0000000..1cc0c1b
> --- /dev/null
> +++ b/scripts/coverity-model.c
> @@ -0,0 +1,170 @@
> +/* Coverity Scan model
> + *
> + * Copyright (C) 2014 Red Hat, Inc.
> + *
> + * Authors:
> + *  Markus Armbruster <armbru@redhat.com>
> + *  Paolo Bonzini <pbonzini@redhat.com>
> + *
> + * This is a modeling file for Coverity Scan. Modeling helps to avoid false
> + * positives.
> + *
> + * - 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
> +#define assert(x) if (!(x)) __coverity_panic__();
> +
> +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, int 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, int 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;
> +
> +    // perhaps __coverity_tainted_data_argument__(buf); for read,
> +    // and __coverity_tainted_data_sink__(buf); for write?
> +    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 */
> +
> +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__((ssize_t) n_bytes);
> +    mem = malloc(n_bytes == 0 ? 1 : n_bytes);
> +    if (!mem) __coverity_panic__ ();
> +    return mem;
> +}

This isn't quite honest: g_malloc(0) yields NULL.  Same for the other
allocation functions.

> +
> +void *
> +g_malloc0 (size_t n_bytes)
> +{
> +    void *mem;
> +    __coverity_negative_sink__((ssize_t) n_bytes);
> +    mem = calloc(1, n_bytes == 0 ? 1 : n_bytes);
> +    if (!mem) __coverity_panic__ ();
> +    return mem;
> +}
> +
> +void g_free (void *mem)
> +{
> +    if (mem) {
> +        free(mem);
> +    }
> +}

Unconditional free(mem) should be fine.

> +
> +void *g_realloc (void * mem, size_t n_bytes)
> +{
> +    __coverity_negative_sink__((ssize_t) n_bytes);
> +    if (n_bytes == 0) {
> +        g_free(mem);
> +        return NULL;
> +    } else {
> +        mem = realloc(mem, n_bytes);
> +        if (!mem) __coverity_panic__ ();
> +        return mem;
> +    }
> +}
> +
> +void *g_try_malloc (size_t n_bytes)
> +{
> +    __coverity_negative_sink__((ssize_t) n_bytes);
> +    return malloc(n_bytes == 0 ? 1 : n_bytes);
> +}
> +
> +void *g_try_malloc0 (size_t n_bytes)
> +{
> +    __coverity_negative_sink__((ssize_t) n_bytes);
> +    return calloc(1, n_bytes == 0 ? 1 : n_bytes);
> +}
> +
> +void *g_try_realloc (void *mem, size_t n_bytes)
> +{
> +    __coverity_negative_sink__((ssize_t) n_bytes);
> +    if (n_bytes == 0) {
> +        g_free(mem);
> +        return NULL;
> +    } else {
> +        return realloc(mem, n_bytes);
> +    }
> +}
> +
> +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__();
> +}

I wonder how much scanning power a derived model for GLib adds.  I'll
find out.

Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan

[Paolo Bonzini]

Il 18/03/2014 19:40, Markus Armbruster ha scritto:
> > +void *
> > +g_malloc (size_t n_bytes)
> > +{
> > +    void *mem;
> > +    __coverity_negative_sink__((ssize_t) n_bytes);
> > +    mem = malloc(n_bytes == 0 ? 1 : n_bytes);
> > +    if (!mem) __coverity_panic__ ();
> > +    return mem;
> > +}
>
> This isn't quite honest: g_malloc(0) yields NULL.  Same for the other
> allocation functions.

Oh, I didn't know that.

It probably would make static analysis a bit less powerful or will 
return more false positives.  The NULL return for realloc (in the "free" 
case) already causes some.  So I'm undecided between a more correct 
model and a more selective one (with a fat comment).

Paolo

Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 18/03/2014 19:40, Markus Armbruster ha scritto:
>> > +void *
>> > +g_malloc (size_t n_bytes)
>> > +{
>> > +    void *mem;
>> > +    __coverity_negative_sink__((ssize_t) n_bytes);
>> > +    mem = malloc(n_bytes == 0 ? 1 : n_bytes);
>> > +    if (!mem) __coverity_panic__ ();
>> > +    return mem;
>> > +}
>>
>> This isn't quite honest: g_malloc(0) yields NULL.  Same for the other
>> allocation functions.
>
> Oh, I didn't know that.
>
> It probably would make static analysis a bit less powerful or will
> return more false positives.  The NULL return for realloc (in the
> "free" case) already causes some.  So I'm undecided between a more
> correct model and a more selective one (with a fat comment).

I can't see how lying to the analyzer could make it more powerful :)
It can, however, suppress false positives.  Scan and find out how many?

Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan

[Kevin Wolf]

Am 18.03.2014 um 18:03 hat Paolo Bonzini geschrieben:
> 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 | 170 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 170 insertions(+)
>  create mode 100644 scripts/coverity-model.c
> 
> diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c
> new file mode 100644
> index 0000000..1cc0c1b
> --- /dev/null
> +++ b/scripts/coverity-model.c
> @@ -0,0 +1,170 @@
> +/* Coverity Scan model
> + *
> + * Copyright (C) 2014 Red Hat, Inc.
> + *
> + * Authors:
> + *  Markus Armbruster <armbru@redhat.com>
> + *  Paolo Bonzini <pbonzini@redhat.com>
> + *
> + * This is a modeling file for Coverity Scan. Modeling helps to avoid false
> + * positives.
> + *
> + * - 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
> + */

Only copyright notice, but no license?

Kevin

Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan

[Paolo Bonzini]

Il 19/03/2014 10:08, Markus Armbruster ha scritto:
>> It probably would make static analysis a bit less powerful or will
>> return more false positives.  The NULL return for realloc (in the
>> "free" case) already causes some.  So I'm undecided between a more
>> correct model and a more selective one (with a fat comment).
>
> I can't see how lying to the analyzer could make it more powerful :)
> It can, however, suppress false positives.  Scan and find out how many?

Full model (g_malloc returns NULL for 0 argument) => 750 defects

Posted model (g_malloc never returns NULL)        => 702 defects
		-59 NULL_RETURNS defects
		 -1 REVERSE_INULL defects
		+12 TAINTED_SCALAR defects

Reduced model (g_realloc never frees)             => 690 defects
		-12 NULL_RETURNS defects

Of course, silly me, I threw away the results of the analysis for the 
full model.  I'll now rerun it and look for false negatives caused by 
the reduced model.

Paolo

Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan

[Paolo Bonzini]

Il 19/03/2014 13:46, Paolo Bonzini ha scritto:
> Il 19/03/2014 10:08, Markus Armbruster ha scritto:
>>> It probably would make static analysis a bit less powerful or will
>>> return more false positives.  The NULL return for realloc (in the
>>> "free" case) already causes some.  So I'm undecided between a more
>>> correct model and a more selective one (with a fat comment).
>>
>> I can't see how lying to the analyzer could make it more powerful :)
>> It can, however, suppress false positives.  Scan and find out how many?
>
> Full model (g_malloc returns NULL for 0 argument) => 750 defects
>
> Posted model (g_malloc never returns NULL)        => 702 defects
>         -59 NULL_RETURNS defects
>          -1 REVERSE_INULL defects
>         +12 TAINTED_SCALAR defects
>
> Reduced model (g_realloc never frees)             => 690 defects
>         -12 NULL_RETURNS defects
>
> Of course, silly me, I threw away the results of the analysis for the
> full model.  I'll now rerun it and look for false negatives caused by
> the reduced model.

For the REVERSE_INULL and TAINTED_SCALAR defects, I don't see why the 
model should make any difference.  The missing REVERSE_INULL becomes a 
false-negative.  The new TAINTED_SCALAR were false negatives.

I checked ~10 of the NULL_RETURNS and they are all false positives. 
Either the argument really cannot be zero, or it is asserted that it is 
not zero before accessing the array, or the array access is within a for 
loop that will never roll if the size was zero.

Examples:

1) gencb_alloc (and a few others have the same idiom) gets a length, 
allocates a block of the given length, and fills in the beginning of 
that block.  It's arguably missing an assertion that the length is 
good-enough.  No reason for this to be tied to NULL_RETURNS, but in 
practice it is.


2) This only gets zero if there is an overflow, since dma->memmap_size 
is initialized to zero.  But Coverity flags it as a possible NULL return:

316  	    dma->memmap = g_realloc(dma->memmap, sizeof(*entry) *
317  	                    (dma->memmap_size + 1));


3) vnc_dpy_cursor_define calls g_malloc0(vd->cursor_msize), which 
returns NULL if the array has size 0.  Coverity complains because 
cursor_get_mono_mask calls memset on the result, but we already rely 
elsewhere on that not happening for len == 0.


I think we're well into diminishing returns, which justifies using the 
less-precise model.

I'm now adding new models for memset/memcpy/memmove/memcmp that check 
for non-zero argument, and see what that improves with respect to the 
full and reduced models.

Paolo

Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan

[Paolo Bonzini]

Il 19/03/2014 14:56, Paolo Bonzini ha scritto:
> Il 19/03/2014 13:46, Paolo Bonzini ha scritto:
>> Il 19/03/2014 10:08, Markus Armbruster ha scritto:
>>>> It probably would make static analysis a bit less powerful or will
>>>> return more false positives.  The NULL return for realloc (in the
>>>> "free" case) already causes some.  So I'm undecided between a more
>>>> correct model and a more selective one (with a fat comment).
>>>
>>> I can't see how lying to the analyzer could make it more powerful :)
>>> It can, however, suppress false positives.  Scan and find out how many?
>>
>> Full model (g_malloc returns NULL for 0 argument) => 750 defects
>>
>> Posted model (g_malloc never returns NULL)        => 702 defects
>>         -59 NULL_RETURNS defects
>>          -1 REVERSE_INULL defects
>>         +12 TAINTED_SCALAR defects
>>
>> Reduced model (g_realloc never frees)             => 690 defects
>>         -12 NULL_RETURNS defects
>>
>> Of course, silly me, I threw away the results of the analysis for the
>> full model.  I'll now rerun it and look for false negatives caused by
>> the reduced model.
>
> For the REVERSE_INULL and TAINTED_SCALAR defects, I don't see why the
> model should make any difference.  The missing REVERSE_INULL becomes a
> false-negative.  The new TAINTED_SCALAR were false negatives.
>
> I checked ~10 of the NULL_RETURNS and they are all false positives.
> Either the argument really cannot be zero, or it is asserted that it is
> not zero before accessing the array, or the array access is within a for
> loop that will never roll if the size was zero.
>
> Examples:
>
> 1) gencb_alloc (and a few others have the same idiom) gets a length,
> allocates a block of the given length, and fills in the beginning of
> that block.  It's arguably missing an assertion that the length is
> good-enough.  No reason for this to be tied to NULL_RETURNS, but in
> practice it is.
>
>
> 2) This only gets zero if there is an overflow, since dma->memmap_size
> is initialized to zero.  But Coverity flags it as a possible NULL return:
>
> 316          dma->memmap = g_realloc(dma->memmap, sizeof(*entry) *
> 317                          (dma->memmap_size + 1));
>
>
> 3) vnc_dpy_cursor_define calls g_malloc0(vd->cursor_msize), which
> returns NULL if the array has size 0.  Coverity complains because
> cursor_get_mono_mask calls memset on the result, but we already rely
> elsewhere on that not happening for len == 0.
>
>
> I think we're well into diminishing returns, which justifies using the
> less-precise model.
>
> I'm now adding new models for memset/memcpy/memmove/memcmp that check
> for non-zero argument, and see what that improves with respect to the
> full and reduced models.

Doing this only fixes one false positive.  Given the results, okay to 
use the limited model where realloc never frees and malloc(0) returns 
non-NULL?

Paolo

Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 19/03/2014 14:56, Paolo Bonzini ha scritto:
>> Il 19/03/2014 13:46, Paolo Bonzini ha scritto:
>>> Il 19/03/2014 10:08, Markus Armbruster ha scritto:
>>>>> It probably would make static analysis a bit less powerful or will
>>>>> return more false positives.  The NULL return for realloc (in the
>>>>> "free" case) already causes some.  So I'm undecided between a more
>>>>> correct model and a more selective one (with a fat comment).
>>>>
>>>> I can't see how lying to the analyzer could make it more powerful :)
>>>> It can, however, suppress false positives.  Scan and find out how many?
>>>
>>> Full model (g_malloc returns NULL for 0 argument) => 750 defects
>>>
>>> Posted model (g_malloc never returns NULL)        => 702 defects
>>>         -59 NULL_RETURNS defects
>>>          -1 REVERSE_INULL defects
>>>         +12 TAINTED_SCALAR defects
>>>
>>> Reduced model (g_realloc never frees)             => 690 defects
>>>         -12 NULL_RETURNS defects
>>>
>>> Of course, silly me, I threw away the results of the analysis for the
>>> full model.  I'll now rerun it and look for false negatives caused by
>>> the reduced model.
>>
>> For the REVERSE_INULL and TAINTED_SCALAR defects, I don't see why the
>> model should make any difference.  The missing REVERSE_INULL becomes a
>> false-negative.  The new TAINTED_SCALAR were false negatives.

REVERSE_INULL is a null check after a dereference.  Pretending
g_malloc() never returns a null pointer can conceivably create false
negatives: Coverity flags a null check as REVERSE_INULL even though it's
necessary.  This should be rare, because checking for null return value
instead of zero size argument is unnatural.

I can't explain offhand how the TAINTED_SCALAR get unmasked.

>> I checked ~10 of the NULL_RETURNS and they are all false positives.
>> Either the argument really cannot be zero, or it is asserted that it is
>> not zero before accessing the array, or the array access is within a for
>> loop that will never roll if the size was zero.

I've seen a few like these myself.  Coverity isn't quite smart enough to
prove non-zero size, and then non-null return value.

>>
>> Examples:
>>
>> 1) gencb_alloc (and a few others have the same idiom) gets a length,
>> allocates a block of the given length, and fills in the beginning of
>> that block.  It's arguably missing an assertion that the length is
>> good-enough.  No reason for this to be tied to NULL_RETURNS, but in
>> practice it is.
>>
>>
>> 2) This only gets zero if there is an overflow, since dma->memmap_size
>> is initialized to zero.  But Coverity flags it as a possible NULL return:
>>
>> 316          dma->memmap = g_realloc(dma->memmap, sizeof(*entry) *
>> 317                          (dma->memmap_size + 1));

g_renew(struct memmap_entry_s, dma->memmap, dma->memmap_size + 1)

One overflow less to worry about.  I'd give sich transformations a try
if getting tree-wide cleanups committed wasn't such an impossible pain.
Oh well, we can fix 'em one CVE at a time.

>> 3) vnc_dpy_cursor_define calls g_malloc0(vd->cursor_msize), which
>> returns NULL if the array has size 0.  Coverity complains because
>> cursor_get_mono_mask calls memset on the result, but we already rely
>> elsewhere on that not happening for len == 0.
>>
>>
>> I think we're well into diminishing returns, which justifies using the
>> less-precise model.
>>
>> I'm now adding new models for memset/memcpy/memmove/memcmp that check
>> for non-zero argument, and see what that improves with respect to the
>> full and reduced models.
>
> Doing this only fixes one false positive.

Coverity comes with models for all four (see its
library/generic/libc/all/all.c).  I'm surprised you adding your own
models has any positive effect.

>                                            Given the results, okay to
> use the limited model where realloc never frees and malloc(0) returns
> non-NULL?

I'd describe realloc() as "always frees the old block, returns a new
block, which is never null".  We might mean the same.

Go ahead with the lying^Wlimited model, with a big, fat comment
explaining our reasons.

Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan

[Paolo Bonzini]

Il 19/03/2014 16:56, Markus Armbruster ha scritto:
>
>> >                                            Given the results, okay to
>> > use the limited model where realloc never frees and malloc(0) returns
>> > non-NULL?
> I'd describe realloc() as "always frees the old block, returns a new
> block, which is never null".  We might mean the same.

Yes, that's what I would do too, but it misses an important difference 
between malloc() and realloc().

"If size is 0, then malloc() returns either NULL, or a unique pointer 
value that can later be successfully passed to free()".  Instead, for 
realloc, "if size is equal to zero, and ptr is not NULL, then the call 
is equivalent to free(ptr)" and presumably must return NULL.

So I'm doubly cheating by giving realloc(foo, 0) free+malloc semantics 
instead of free.

BTW, this means that the handy "malloc(0) really means malloc(1)" 
semantics are inconsistent because you cannot implement realloc(foo, 0) 
that way.

Paolo

> Go ahead with the lying^Wlimited model, with a big, fat comment
> explaining our reasons.