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