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

[Markus Armbruster]

Paolo Bonzini <address@hidden> 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.