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

[Paolo Bonzini <pbonzini@redhat.com>]

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