qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH v2] scripts: add sample model file for Coverity Scan
Date: Thu, 20 Mar 2014 14:01:18 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]