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

[Qemu-devel] [PATCH v2] 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>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-model.c | 178 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 178 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..6a1dd6d
--- /dev/null
+++ b/scripts/coverity-model.c
@@ -0,0 +1,178 @@
+/* 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 a modeling file for Coverity Scan. Modeling helps to avoid false
+ * positives and in some cases helps Coverity in reporting more defects,
+ * especially memory leaks.
+ *
+ * - 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 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 would really never be dereferenced 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__((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);
+    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__((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);
+    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__();
+}
-- 
1.8.5.3

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]

On 03/19/2014 10:52 AM, 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).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Double S-o-B looks odd.


> + *
> + * 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.

Aren't the license and authors blurbs usually in the other order?


> +
> +#define NULL (void *)0

Missing ()

> +#define assert(x) if (!(x)) __coverity_panic__();

Will this break any 'if () assert(); else {}' blocks?  Obviously, such
blocks already violate coding convention, but you might as well make
this definition safe to use for older code.

> +
> +static void __write(uint8_t *buf, int len)

Will the fact that you used 'int len' instead of 'size_t' bite us on 32-
vs. 64-bit?  Same for __read.


> +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__ ();

Is it worth being consistent on spacing before (?

> +void g_free (void *mem)
> +{
> +    if (mem) {
> +        free(mem);
> +    }

Doesn't coverity already know that free(NULL) is a no-op, without you
having to repeat it?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Paolo Bonzini]

Il 19/03/2014 18:32, Eric Blake ha scritto:
>> + *
>> + * 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.
>
> Aren't the license and authors blurbs usually in the other order?

Not in the sample I copied from (migration.c).

>
>> +#define assert(x) if (!(x)) __coverity_panic__();
>
> Will this break any 'if () assert(); else {}' blocks?  Obviously, such
> blocks already violate coding convention, but you might as well make
> this definition safe to use for older code.

Ok.

>> +
>> +static void __write(uint8_t *buf, int len)
>
> Will the fact that you used 'int len' instead of 'size_t' bite us on 32-
> vs. 64-bit?  Same for __read.

Yeah, I copied this from address_space_rw.  I'll change to ssize_t to 
catch negative values.

>
>> +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__ ();
>
> Is it worth being consistent on spacing before (?

Yes.

>> +void g_free (void *mem)
>> +{
>> +    if (mem) {
>> +        free(mem);
>> +    }
>
> Doesn't coverity already know that free(NULL) is a no-op, without you
> having to repeat it?

This part came from Markus. :)

Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 19/03/2014 18:32, Eric Blake ha scritto:
>>> + *
>>> + * 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.
>>
>> Aren't the license and authors blurbs usually in the other order?
>
> Not in the sample I copied from (migration.c).
>
>>
>>> +#define assert(x) if (!(x)) __coverity_panic__();
>>
>> Will this break any 'if () assert(); else {}' blocks?  Obviously, such
>> blocks already violate coding convention, but you might as well make
>> this definition safe to use for older code.
>
> Ok.

Coverity's builtin model models assert already, see
library/generic/libc/all/all.c.  We shouldn't override it without a good
reason.

>>> +
>>> +static void __write(uint8_t *buf, int len)
>>
>> Will the fact that you used 'int len' instead of 'size_t' bite us on 32-
>> vs. 64-bit?  Same for __read.
>
> Yeah, I copied this from address_space_rw.  I'll change to ssize_t to
> catch negative values.

Change the real address_space_rw(), or the model's __write()?

>>> +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__ ();
>>
>> Is it worth being consistent on spacing before (?
>
> Yes.
>
>>> +void g_free (void *mem)
>>> +{
>>> +    if (mem) {
>>> +        free(mem);
>>> +    }
>>
>> Doesn't coverity already know that free(NULL) is a no-op, without you
>> having to repeat it?
>
> This part came from Markus. :)

I don't know what possessed me when I wrote (or copied?) the silly
conditional.  Let's drop it before someone else copies it.

Re: [Qemu-devel] [PATCH v2] 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>
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/coverity-model.c | 178 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 178 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..6a1dd6d
> --- /dev/null
> +++ b/scripts/coverity-model.c
> @@ -0,0 +1,178 @@
> +/* 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 a modeling file for Coverity Scan. Modeling helps to avoid false
> + * positives and in some cases helps Coverity in reporting more defects,
> + * especially memory leaks.

Suggest:

/*
 * 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.
 */

This should make it clear that user models is only about functions that
cause scanning trouble.

Could perhaps be put into the commit message, too.

> + *
> + * - 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__();

See Eric's comments, and my reply.

> +
> +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;

These typedefs break when scanning on an oddball system.  Let's not
worry about that now.

> +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?

Suggest to mark things you'd like to try with TODO for easy recogniztion
by humans and grep.

> +    if (is_write) __write(buf, len); else __read(buf, len);
> +
> +    return result;
> +}

I'm curious: could you give me a rough idea on how modelling
address_space_rw() affects results?

> +
> +/* 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;
> +    }
> +}

Curious again: is this just insurance, or did you observe scanning
improvements?

> +
> +/* 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 would really never be dereferenced anyway.

Suggest to add "in the vast majority of cases".  It *is* possible that
we suppress a defect report for an actual (and invalid!) null pointer
dereference here, we just believe it's too unlikely to be worth wading
through the false positives.

> + */
> +
> +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);

Judging from Coverity's builtin model for malloc(), the cast is
superfluous.  Please drop it.

> +    mem = malloc(n_bytes == 0 ? 1 : n_bytes);
> +    if (!mem) __coverity_panic__ ();
> +    return mem;
> +}

This claims g_malloc(0) returns a non-null pointer to a block of size 1.
Could we say it returns a non-null pointer to a block of size 0?

    int success;

    __coverity_negative_sink__(size);

    if (success) {
        void* tmp = __coverity_alloc__(size);
        if (tmp) __coverity_mark_as_uninitialized_buffer__(tmp);
        __coverity_mark_as_afm_allocated__(tmp, AFM_free);
        return tmp;
    } else {
        __coverity_panic__ ();
    }

> +
> +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);
> +    }
> +}

Let's drop the silly conditional.

> +
> +void *g_realloc (void * mem, size_t n_bytes)
> +{
> +    __coverity_negative_sink__((ssize_t) 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__((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);
> +    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__();
> +}

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

[Paolo Bonzini]

Il 20/03/2014 08:32, Markus Armbruster ha scritto:
>>>> +static void __write(uint8_t *buf, int len)
>>>
>>> Will the fact that you used 'int len' instead of 'size_t' bite us on 32-
>>> vs. 64-bit?  Same for __read.
>>
>> Yeah, I copied this from address_space_rw.  I'll change to ssize_t to
>> catch negative values.
>
> Change the real address_space_rw(), or the model's __write()?

__read and __write for now (hard freeze etc. etc.).

>> +    if (is_write) __write(buf, len); else __read(buf, len);
>> +
>> +    return result;
>> +}
>
> I'm curious: could you give me a rough idea on how modelling
> address_space_rw() affects results?

Sure!  The problematic code is this one:

             if (!memory_access_is_direct(mr, is_write)) {
                 l = memory_access_size(mr, l, addr1);
                 /* XXX: could force current_cpu to NULL to avoid
                    potential bugs */
                 switch (l) {
                 case 8:
                     /* 64 bit write access */
                     val = ldq_p(buf);
                     error |= io_mem_write(mr, addr1, val, 8);
                     break;

Coverity doesn't understand that memory_access_size return a value that 
is less than l, and thus thinks that address_space_rw can do an 8-byte 
access.  So it flags cases where we use it to read into an int or a 
similarly small char[].

It's actually fairly common, it occurs ~20 times.

>> +static int get_keysym(const name2keysym_t *table,
>> +                      const char *name)
>
> Curious again: is this just insurance, or did you observe scanning
> improvements?

It fixes exactly one error.  All of the "tainted value" can be 
considered false positives, but I wanted to have an example on how to 
shut them up.

> This claims g_malloc(0) returns a non-null pointer to a block of size 1.
> Could we say it returns a non-null pointer to a block of size 0?

Not sure of the semantics of __coverity_alloc__(0).  Leave it to further 
future improvements?

>     if (success) {
>         void* tmp = __coverity_alloc__(size);
>         if (tmp) __coverity_mark_as_uninitialized_buffer__(tmp);
>         __coverity_mark_as_afm_allocated__(tmp, AFM_free);
>         return tmp;
>     } else {
>         __coverity_panic__ ();
>     }

Is the "if" needed at all?  The "else" path is killed altogether by 
__coverity_panic__().

Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 20/03/2014 08:32, Markus Armbruster ha scritto:
>>>>> +static void __write(uint8_t *buf, int len)
>>>>
>>>> Will the fact that you used 'int len' instead of 'size_t' bite us on 32-
>>>> vs. 64-bit?  Same for __read.
>>>
>>> Yeah, I copied this from address_space_rw.  I'll change to ssize_t to
>>> catch negative values.
>>
>> Change the real address_space_rw(), or the model's __write()?
>
> __read and __write for now (hard freeze etc. etc.).

Okay.  But let's use size_t instead of ssize_t.  You can catch negative
values with __coverity_negative_sink__() all the same.

>>> +    if (is_write) __write(buf, len); else __read(buf, len);
>>> +
>>> +    return result;
>>> +}
>>
>> I'm curious: could you give me a rough idea on how modelling
>> address_space_rw() affects results?
>
> Sure!  The problematic code is this one:
>
>             if (!memory_access_is_direct(mr, is_write)) {
>                 l = memory_access_size(mr, l, addr1);
>                 /* XXX: could force current_cpu to NULL to avoid
>                    potential bugs */
>                 switch (l) {
>                 case 8:
>                     /* 64 bit write access */
>                     val = ldq_p(buf);
>                     error |= io_mem_write(mr, addr1, val, 8);
>                     break;
>
> Coverity doesn't understand that memory_access_size return a value
> that is less than l, and thus thinks that address_space_rw can do an
> 8-byte access.  So it flags cases where we use it to read into an int
> or a similarly small char[].
>
> It's actually fairly common, it occurs ~20 times.

I see.  Addressing the false positives' root cause with a model feels
worthwhile, in particular since more such uses of address_space_rw() can
pop up any time.

>>> +static int get_keysym(const name2keysym_t *table,
>>> +                      const char *name)
>>
>> Curious again: is this just insurance, or did you observe scanning
>> improvements?
>
> It fixes exactly one error.  All of the "tainted value" can be
> considered false positives, but I wanted to have an example on how to
> shut them up.

After a bit of digging, I think I now understand what your model
asserts: get_keysym() returns either zero or a positive value, and the
positive value is "safe" in a sense that isn't specified further.  This
shuts up the TAINTED_SCALAR defects when the value is used as subscript
in add_keysym().  Correct?

>> This claims g_malloc(0) returns a non-null pointer to a block of size 1.
>> Could we say it returns a non-null pointer to a block of size 0?
>
> Not sure of the semantics of __coverity_alloc__(0).  Leave it to
> further future improvements?

Yes, but with a comment explaining the model's inaccuracy.  Perhaps
something like this:

void *
g_malloc (size_t n_bytes)
{
    void *mem;
    __coverity_negative_sink__((ssize_t) n_bytes);
    /*
     * Mapping size 0 to 1 to ensure result isn't null.  This is a
     * bald-faced lie.  In reality, size 0 returns null, but modelling
     * that correctly produces too many annoying false positives.
     * Would be nice to improve the lie from "size 0 returns a pointer
     * to a block of size 1" to "... of size 0".
     */
    mem = malloc(n_bytes == 0 ? 1 : n_bytes);
    if (!mem) __coverity_panic__ ();
    return mem;
}

Hmm, I wonder whether this less elaborate lie would do, too:

void *
g_malloc (size_t n_bytes)
{
    void *mem;
    __coverity_negative_sink__((ssize_t) n_bytes);
    mem = malloc(n_bytes);
    if (!mem) {
        /*
         * This is a bald-faced lie.  In reality, size 0 returns null,
         * but modelling that correctly produces too many annoying false
         * positives.
         */
        __coverity_panic__ ();
    }
    return mem;
}

>>     if (success) {
>>         void* tmp = __coverity_alloc__(size);
>>         if (tmp) __coverity_mark_as_uninitialized_buffer__(tmp);
>>         __coverity_mark_as_afm_allocated__(tmp, AFM_free);
>>         return tmp;
>>     } else {
>>         __coverity_panic__ ();
>>     }
>
> Is the "if" needed at all?  The "else" path is killed altogether by
> __coverity_panic__().

It tells Coverity that g_malloc() can terminate the process.  Whether
that's useful information I can't say.