[PATCH 0/6] Updates for Coverity modeling file

[PATCH 0/6] Updates for Coverity modeling file

[Paolo Bonzini]

Recently, an update to the Coverity tools caused it to introduce hundreds
of new complaints about using g_free() to free memory areas allocated
by GLib functions.  The solution adopted here (patch 2) is to just
make g_free a synonym of free, removing the custom g_free marker from
__coverity_mark_as_afm_allocated__ and __coverity_mark_as_afm_freed__.
This unfortunately goes against the GLib documentation, which suggests
that g_malloc() should be matched with g_free() and plain malloc() with
free(); since GLib 2.46 however g_malloc() is hardcoded to always use the
system malloc implementation, and g_free is just "free" plus a tracepoint.
Therefore, this should not cause any problem in practice.

There are still problems, in that Coverity believes that the result of
g_malloc/g_malloc0 can return NULL, which is not true.  What caused the
issue is anybody's guess; possibly a new version of Coverity changed
the semantics of __coverity_alloc__, but I also had to inline the model
of g_malloc_n in g_malloc (and likewise for the other five functions)
though it seems like Coverity.  This is implemented in patches 5-6.

On top of these changes, this includes a few more changes to the model
file:

- patch 1 include a few more simplified memory read/write models, so
  that Coverity has a model for all functions in the pci_dma_* and
  dma_memory_* family.  This fixes a few incorrect out of bounds
  false positive, where Coverity does not realize that only up to
  LEN bytes are read/written by those functions

- patch 3 removes the model for various allocation functions, which
  is unnecessary now that we need not (or cannot) detect their
  being paired with g_free

- patch 4 is a small cleanup that makes the inlined allocation
  functions smaller.

This series is a sort of FYI; since the only way to debug the model file
is to upload it to scan.coverity.com, these changes are all already live.
The last will be as of the next build, but was effective last Thursday
and worked (I tried disabling it on Friday in something like a bisection,
but it failed and I have now reverted to Thursday's model).

Thanks,

Paolo

Paolo Bonzini (6):
  coverity-model: update address_space_read/write models
  coverity-model: make g_free a synonym of free
  coverity-model: remove model for more allocation functions
  coverity-model: clean up the models for array allocation functions
  coverity-model: constrain g_malloc/g_malloc0/g_realloc as never
    returning NULL
  coverity-model: write models fully for non-array allocation functions

 scripts/coverity-scan/model.c | 235 ++++++++++++++++------------------
 1 file changed, 110 insertions(+), 125 deletions(-)

-- 
2.31.1

[PATCH 1/6] coverity-model: update address_space_read/write models

[Paolo Bonzini]

Use void * for consistency with the actual function; provide a model
for MemoryRegionCache functions and for address_space_rw.  These
let Coverity understand the bounds of the data that various functions
read and write even at very high levels of inlining (e.g. pci_dma_read).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-scan/model.c | 48 ++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c
index 2c0346ff25..e1bdf0ad84 100644
--- a/scripts/coverity-scan/model.c
+++ b/scripts/coverity-scan/model.c
@@ -45,9 +45,10 @@ typedef struct va_list_str *va_list;
 /* exec.c */
 
 typedef struct AddressSpace AddressSpace;
+typedef struct MemoryRegionCache MemoryRegionCache;
 typedef uint64_t hwaddr;
 typedef uint32_t MemTxResult;
-typedef uint64_t MemTxAttrs;
+typedef struct MemTxAttrs {} MemTxAttrs;
 
 static void __bufwrite(uint8_t *buf, ssize_t len)
 {
@@ -67,9 +68,40 @@ static void __bufread(uint8_t *buf, ssize_t len)
     int last = buf[len-1];
 }
 
+MemTxResult address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
+                                      MemTxAttrs attrs,
+                                      void *buf, int len)
+{
+    MemTxResult result;
+    // TODO: investigate impact of treating reads as producing
+    // tainted data, with __coverity_tainted_data_argument__(buf).
+    __bufwrite(buf, len);
+    return result;
+}
+
+MemTxResult address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
+                                MemTxAttrs attrs,
+                                const void *buf, int len)
+{
+    MemTxResult result;
+    __bufread(buf, len);
+    return result;
+}
+
+MemTxResult address_space_rw_cached(MemoryRegionCache *cache, hwaddr addr,
+                                    MemTxAttrs attrs,
+                                    void *buf, int len, bool is_write)
+{
+    if (is_write) {
+        return address_space_write_cached(cache, addr, attrs, buf, len);
+    } else {
+        return address_space_read_cached(cache, addr, attrs, buf, len);
+    }
+}
+
 MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
                                MemTxAttrs attrs,
-                               uint8_t *buf, int len)
+                               void *buf, int len)
 {
     MemTxResult result;
     // TODO: investigate impact of treating reads as producing
@@ -80,13 +112,23 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
 
 MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
                                 MemTxAttrs attrs,
-                                const uint8_t *buf, int len)
+                                const void *buf, int len)
 {
     MemTxResult result;
     __bufread(buf, len);
     return result;
 }
 
+MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
+                             MemTxAttrs attrs,
+                             void *buf, int len, bool is_write)
+{
+    if (is_write) {
+        return address_space_write(as, addr, attrs, buf, len);
+    } else {
+        return address_space_read(as, addr, attrs, buf, len);
+    }
+}
 
 /* Tainting */
 
-- 
2.31.1

[PATCH 2/6] coverity-model: make g_free a synonym of free

[Paolo Bonzini]

Recently, Coverity has started complaining about using g_free() to free
memory areas allocated by GLib functions not included in model.c,
such as g_strfreev.  This unfortunately goes against the GLib
documentation, which suggests that g_malloc() should be matched
with g_free() and plain malloc() with free(); since GLib 2.46 however
g_malloc() is hardcoded to always use the system malloc implementation,
and g_free is just "free" plus a tracepoint.  Therefore, this
should not cause any problem in practice.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-scan/model.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c
index e1bdf0ad84..8e64a84c5a 100644
--- a/scripts/coverity-scan/model.c
+++ b/scripts/coverity-scan/model.c
@@ -186,7 +186,7 @@ void *g_malloc_n(size_t nmemb, size_t size)
     sz = nmemb * size;
     ptr = __coverity_alloc__(sz);
     __coverity_mark_as_uninitialized_buffer__(ptr);
-    __coverity_mark_as_afm_allocated__(ptr, "g_free");
+    __coverity_mark_as_afm_allocated__(ptr, AFM_free);
     return ptr;
 }
 
@@ -200,7 +200,7 @@ void *g_malloc0_n(size_t nmemb, size_t size)
     sz = nmemb * size;
     ptr = __coverity_alloc__(sz);
     __coverity_writeall0__(ptr);
-    __coverity_mark_as_afm_allocated__(ptr, "g_free");
+    __coverity_mark_as_afm_allocated__(ptr, AFM_free);
     return ptr;
 }
 
@@ -218,14 +218,14 @@ void *g_realloc_n(void *ptr, size_t nmemb, size_t size)
      * model that.  See Coverity's realloc() model
      */
     __coverity_writeall__(ptr);
-    __coverity_mark_as_afm_allocated__(ptr, "g_free");
+    __coverity_mark_as_afm_allocated__(ptr, AFM_free);
     return ptr;
 }
 
 void g_free(void *ptr)
 {
     __coverity_free__(ptr);
-    __coverity_mark_as_afm_freed__(ptr, "g_free");
+    __coverity_mark_as_afm_freed__(ptr, AFM_free);
 }
 
 /*
@@ -328,7 +328,7 @@ char *g_strdup(const char *s)
     __coverity_string_null_sink__(s);
     __coverity_string_size_sink__(s);
     dup = __coverity_alloc_nosize__();
-    __coverity_mark_as_afm_allocated__(dup, "g_free");
+    __coverity_mark_as_afm_allocated__(dup, AFM_free);
     for (i = 0; (dup[i] = s[i]); i++) ;
     return dup;
 }
@@ -362,7 +362,7 @@ char *g_strdup_printf(const char *format, ...)
 
     s = __coverity_alloc_nosize__();
     __coverity_writeall__(s);
-    __coverity_mark_as_afm_allocated__(s, "g_free");
+    __coverity_mark_as_afm_allocated__(s, AFM_free);
     return s;
 }
 
@@ -375,11 +375,10 @@ char *g_strdup_vprintf(const char *format, va_list ap)
     __coverity_string_size_sink__(format);
 
     ch = *format;
-    ch = *(char *)ap;
 
     s = __coverity_alloc_nosize__();
     __coverity_writeall__(s);
-    __coverity_mark_as_afm_allocated__(s, "g_free");
+    __coverity_mark_as_afm_allocated__(s, AFM_free);
 
     return len;
 }
@@ -395,7 +394,7 @@ char *g_strconcat(const char *s, ...)
 
     s = __coverity_alloc_nosize__();
     __coverity_writeall__(s);
-    __coverity_mark_as_afm_allocated__(s, "g_free");
+    __coverity_mark_as_afm_allocated__(s, AFM_free);
     return s;
 }
 
-- 
2.31.1

[PATCH 3/6] coverity-model: remove model for more allocation functions

[Paolo Bonzini]

These models are not needed anymore now that Coverity does not check
anymore that the result is used with "g_free".  Coverity understands
GCC attributes and uses them to detect leaks.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-scan/model.c | 105 +---------------------------------
 1 file changed, 1 insertion(+), 104 deletions(-)

diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c
index 8e64a84c5a..1a5f39d2ae 100644
--- a/scripts/coverity-scan/model.c
+++ b/scripts/coverity-scan/model.c
@@ -263,7 +263,7 @@ void *g_try_realloc_n(void *ptr, size_t nmemb, size_t size)
     return g_realloc_n(ptr, nmemb, size);
 }
 
-/* Trivially derive the g_FOO() from the g_FOO_n() */
+/* Derive the g_FOO() from the g_FOO_n() */
 
 void *g_malloc(size_t size)
 {
@@ -295,109 +295,6 @@ void *g_try_realloc(void *ptr, size_t size)
     return g_try_realloc_n(ptr, 1, size);
 }
 
-/* Other memory allocation functions */
-
-void *g_memdup(const void *ptr, unsigned size)
-{
-    unsigned char *dup;
-    unsigned i;
-
-    if (!ptr) {
-        return NULL;
-    }
-
-    dup = g_malloc(size);
-    for (i = 0; i < size; i++)
-        dup[i] = ((unsigned char *)ptr)[i];
-    return dup;
-}
-
-/*
- * GLib string allocation functions
- */
-
-char *g_strdup(const char *s)
-{
-    char *dup;
-    size_t i;
-
-    if (!s) {
-        return NULL;
-    }
-
-    __coverity_string_null_sink__(s);
-    __coverity_string_size_sink__(s);
-    dup = __coverity_alloc_nosize__();
-    __coverity_mark_as_afm_allocated__(dup, AFM_free);
-    for (i = 0; (dup[i] = s[i]); i++) ;
-    return dup;
-}
-
-char *g_strndup(const char *s, size_t n)
-{
-    char *dup;
-    size_t i;
-
-    __coverity_negative_sink__(n);
-
-    if (!s) {
-        return NULL;
-    }
-
-    dup = g_malloc(n + 1);
-    for (i = 0; i < n && (dup[i] = s[i]); i++) ;
-    dup[i] = 0;
-    return dup;
-}
-
-char *g_strdup_printf(const char *format, ...)
-{
-    char ch, *s;
-    size_t len;
-
-    __coverity_string_null_sink__(format);
-    __coverity_string_size_sink__(format);
-
-    ch = *format;
-
-    s = __coverity_alloc_nosize__();
-    __coverity_writeall__(s);
-    __coverity_mark_as_afm_allocated__(s, AFM_free);
-    return s;
-}
-
-char *g_strdup_vprintf(const char *format, va_list ap)
-{
-    char ch, *s;
-    size_t len;
-
-    __coverity_string_null_sink__(format);
-    __coverity_string_size_sink__(format);
-
-    ch = *format;
-
-    s = __coverity_alloc_nosize__();
-    __coverity_writeall__(s);
-    __coverity_mark_as_afm_allocated__(s, AFM_free);
-
-    return len;
-}
-
-char *g_strconcat(const char *s, ...)
-{
-    char *s;
-
-    /*
-     * Can't model: last argument must be null, the others
-     * null-terminated strings
-     */
-
-    s = __coverity_alloc_nosize__();
-    __coverity_writeall__(s);
-    __coverity_mark_as_afm_allocated__(s, AFM_free);
-    return s;
-}
-
 /* Other glib functions */
 
 typedef struct pollfd GPollFD;
-- 
2.31.1

[PATCH 4/6] coverity-model: clean up the models for array allocation functions

[Paolo Bonzini]

sz is only used in one place, so replace it with nmemb * size in
that one place.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-scan/model.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c
index 1a5f39d2ae..2d384bdd79 100644
--- a/scripts/coverity-scan/model.c
+++ b/scripts/coverity-scan/model.c
@@ -178,13 +178,11 @@ uint8_t replay_get_byte(void)
 
 void *g_malloc_n(size_t nmemb, size_t size)
 {
-    size_t sz;
     void *ptr;
 
     __coverity_negative_sink__(nmemb);
     __coverity_negative_sink__(size);
-    sz = nmemb * size;
-    ptr = __coverity_alloc__(sz);
+    ptr = __coverity_alloc__(nmemb * size);
     __coverity_mark_as_uninitialized_buffer__(ptr);
     __coverity_mark_as_afm_allocated__(ptr, AFM_free);
     return ptr;
@@ -192,13 +190,11 @@ void *g_malloc_n(size_t nmemb, size_t size)
 
 void *g_malloc0_n(size_t nmemb, size_t size)
 {
-    size_t sz;
     void *ptr;
 
     __coverity_negative_sink__(nmemb);
     __coverity_negative_sink__(size);
-    sz = nmemb * size;
-    ptr = __coverity_alloc__(sz);
+    ptr = __coverity_alloc__(nmemb * size);
     __coverity_writeall0__(ptr);
     __coverity_mark_as_afm_allocated__(ptr, AFM_free);
     return ptr;
@@ -206,13 +202,10 @@ void *g_malloc0_n(size_t nmemb, size_t size)
 
 void *g_realloc_n(void *ptr, size_t nmemb, size_t size)
 {
-    size_t sz;
-
     __coverity_negative_sink__(nmemb);
     __coverity_negative_sink__(size);
-    sz = nmemb * size;
     __coverity_escape__(ptr);
-    ptr = __coverity_alloc__(sz);
+    ptr = __coverity_alloc__(nmemb * size);
     /*
      * Memory beyond the old size isn't actually initialized.  Can't
      * model that.  See Coverity's realloc() model
-- 
2.31.1

[PATCH 5/6] coverity-model: constrain g_malloc/g_malloc0/g_realloc as never returning NULL

[Paolo Bonzini]

g_malloc/g_malloc0/g_realloc only return NULL if the size is 0; we do not need
to cover that in the model, and so far have expected __coverity_alloc__
to model a non-NULL return value.  But that apparently does not work
anymore, so add some extra conditionals that invoke __coverity_panic__
for NULL pointers.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-scan/model.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c
index 2d384bdd79..028f13e9e3 100644
--- a/scripts/coverity-scan/model.c
+++ b/scripts/coverity-scan/model.c
@@ -183,6 +183,9 @@ void *g_malloc_n(size_t nmemb, size_t size)
     __coverity_negative_sink__(nmemb);
     __coverity_negative_sink__(size);
     ptr = __coverity_alloc__(nmemb * size);
+    if (!ptr) {
+        __coverity_panic__();
+    }
     __coverity_mark_as_uninitialized_buffer__(ptr);
     __coverity_mark_as_afm_allocated__(ptr, AFM_free);
     return ptr;
@@ -195,6 +198,9 @@ void *g_malloc0_n(size_t nmemb, size_t size)
     __coverity_negative_sink__(nmemb);
     __coverity_negative_sink__(size);
     ptr = __coverity_alloc__(nmemb * size);
+    if (!ptr) {
+        __coverity_panic__();
+    }
     __coverity_writeall0__(ptr);
     __coverity_mark_as_afm_allocated__(ptr, AFM_free);
     return ptr;
@@ -206,6 +212,9 @@ void *g_realloc_n(void *ptr, size_t nmemb, size_t size)
     __coverity_negative_sink__(size);
     __coverity_escape__(ptr);
     ptr = __coverity_alloc__(nmemb * size);
+    if (!ptr) {
+        __coverity_panic__();
+    }
     /*
      * Memory beyond the old size isn't actually initialized.  Can't
      * model that.  See Coverity's realloc() model
-- 
2.31.1

[PATCH 6/6] coverity-model: write models fully for non-array allocation functions

[Paolo Bonzini]

Coverity seems to have issues figuring out the properties of g_malloc0
and other non *_n functions.  While this was "fixed" by removing the
custom second argument to __coverity_mark_as_afm_allocated__, inline
the code from the array-based allocation functions to avoid future
issues.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-scan/model.c | 57 +++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 6 deletions(-)

diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c
index 028f13e9e3..9d4fba53d9 100644
--- a/scripts/coverity-scan/model.c
+++ b/scripts/coverity-scan/model.c
@@ -269,32 +269,77 @@ void *g_try_realloc_n(void *ptr, size_t nmemb, size_t size)
 
 void *g_malloc(size_t size)
 {
-    return g_malloc_n(1, size);
+    void *ptr;
+
+    __coverity_negative_sink__(size);
+    ptr = __coverity_alloc__(size);
+    if (!ptr) {
+        __coverity_panic__();
+    }
+    __coverity_mark_as_uninitialized_buffer__(ptr);
+    __coverity_mark_as_afm_allocated__(ptr, AFM_free);
+    return ptr;
 }
 
 void *g_malloc0(size_t size)
 {
-    return g_malloc0_n(1, size);
+    void *ptr;
+
+    __coverity_negative_sink__(size);
+    ptr = __coverity_alloc__(size);
+    if (!ptr) {
+        __coverity_panic__();
+    }
+    __coverity_writeall0__(ptr);
+    __coverity_mark_as_afm_allocated__(ptr, AFM_free);
+    return ptr;
 }
 
 void *g_realloc(void *ptr, size_t size)
 {
-    return g_realloc_n(ptr, 1, size);
+    __coverity_negative_sink__(size);
+    __coverity_escape__(ptr);
+    ptr = __coverity_alloc__(size);
+    if (!ptr) {
+        __coverity_panic__();
+    }
+    /*
+     * Memory beyond the old size isn't actually initialized.  Can't
+     * model that.  See Coverity's realloc() model
+     */
+    __coverity_writeall__(ptr);
+    __coverity_mark_as_afm_allocated__(ptr, AFM_free);
+    return ptr;
 }
 
 void *g_try_malloc(size_t size)
 {
-    return g_try_malloc_n(1, size);
+    int nomem;
+
+    if (nomem) {
+        return NULL;
+    }
+    return g_malloc(size);
 }
 
 void *g_try_malloc0(size_t size)
 {
-    return g_try_malloc0_n(1, size);
+    int nomem;
+
+    if (nomem) {
+        return NULL;
+    }
+    return g_malloc0(size);
 }
 
 void *g_try_realloc(void *ptr, size_t size)
 {
-    return g_try_realloc_n(ptr, 1, size);
+    int nomem;
+
+    if (nomem) {
+        return NULL;
+    }
+    return g_realloc(ptr, size);
 }
 
 /* Other glib functions */
-- 
2.31.1

Re: [PATCH 1/6] coverity-model: update address_space_read/write models

[Peter Maydell]

On Sat, 31 Jul 2021 at 07:29, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Use void * for consistency with the actual function; provide a model
> for MemoryRegionCache functions and for address_space_rw.  These
> let Coverity understand the bounds of the data that various functions
> read and write even at very high levels of inlining (e.g. pci_dma_read).
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 2/6] coverity-model: make g_free a synonym of free

[Peter Maydell]

On Sat, 31 Jul 2021 at 07:29, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Recently, Coverity has started complaining about using g_free() to free
> memory areas allocated by GLib functions not included in model.c,
> such as g_strfreev.  This unfortunately goes against the GLib
> documentation, which suggests that g_malloc() should be matched
> with g_free() and plain malloc() with free(); since GLib 2.46 however
> g_malloc() is hardcoded to always use the system malloc implementation,
> and g_free is just "free" plus a tracepoint.  Therefore, this
> should not cause any problem in practice.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

A bit sad to lose the distinction, but as you say the mismatch is now
guaranteed by glib to not be a bug, and in any case we already managed
to get most of the errors out of our codebase.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 3/6] coverity-model: remove model for more allocation functions

[Peter Maydell]

On Sat, 31 Jul 2021 at 07:29, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> These models are not needed anymore now that Coverity does not check
> anymore that the result is used with "g_free".  Coverity understands
> GCC attributes and uses them to detect leaks.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/coverity-scan/model.c | 105 +---------------------------------
>  1 file changed, 1 insertion(+), 104 deletions(-)
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 4/6] coverity-model: clean up the models for array allocation functions

[Peter Maydell]

On Sat, 31 Jul 2021 at 07:31, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> sz is only used in one place, so replace it with nmemb * size in
> that one place.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/coverity-scan/model.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c
> index 1a5f39d2ae..2d384bdd79 100644
> --- a/scripts/coverity-scan/model.c
> +++ b/scripts/coverity-scan/model.c
> @@ -178,13 +178,11 @@ uint8_t replay_get_byte(void)
>
>  void *g_malloc_n(size_t nmemb, size_t size)
>  {
> -    size_t sz;
>      void *ptr;
>
>      __coverity_negative_sink__(nmemb);
>      __coverity_negative_sink__(size);
> -    sz = nmemb * size;
> -    ptr = __coverity_alloc__(sz);
> +    ptr = __coverity_alloc__(nmemb * size);
>      __coverity_mark_as_uninitialized_buffer__(ptr);
>      __coverity_mark_as_afm_allocated__(ptr, AFM_free);
>      return ptr;

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

The real g_malloc_n() returns failure if the multiplication
would overflow; I guess Coverity currently doesn't have any
warnings it generates as a result of assuming overflow
might happen?

thanks
-- PMM

Re: [PATCH 5/6] coverity-model: constrain g_malloc/g_malloc0/g_realloc as never returning NULL

[Peter Maydell]

On Sat, 31 Jul 2021 at 07:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> g_malloc/g_malloc0/g_realloc only return NULL if the size is 0; we do not need
> to cover that in the model, and so far have expected __coverity_alloc__
> to model a non-NULL return value.  But that apparently does not work
> anymore, so add some extra conditionals that invoke __coverity_panic__
> for NULL pointers.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I do wish Coverity Scan had a better notification of updates/changes
and feedback path for bugs than "we'll just silently break stuff for
you" :-(

thanks
-- PMM

Re: [PATCH 6/6] coverity-model: write models fully for non-array allocation functions

[Peter Maydell]

On Sat, 31 Jul 2021 at 07:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Coverity seems to have issues figuring out the properties of g_malloc0
> and other non *_n functions.  While this was "fixed" by removing the
> custom second argument to __coverity_mark_as_afm_allocated__, inline
> the code from the array-based allocation functions to avoid future
> issues.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 0/6] Updates for Coverity modeling file

[Peter Maydell]

On Sat, 31 Jul 2021 at 07:29, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Recently, an update to the Coverity tools caused it to introduce hundreds
> of new complaints about using g_free() to free memory areas allocated
> by GLib functions.  The solution adopted here (patch 2) is to just
> make g_free a synonym of free, removing the custom g_free marker from
> __coverity_mark_as_afm_allocated__ and __coverity_mark_as_afm_freed__.
> This unfortunately goes against the GLib documentation, which suggests
> that g_malloc() should be matched with g_free() and plain malloc() with
> free(); since GLib 2.46 however g_malloc() is hardcoded to always use the
> system malloc implementation, and g_free is just "free" plus a tracepoint.
> Therefore, this should not cause any problem in practice.
>
> There are still problems, in that Coverity believes that the result of
> g_malloc/g_malloc0 can return NULL, which is not true.  What caused the
> issue is anybody's guess; possibly a new version of Coverity changed
> the semantics of __coverity_alloc__, but I also had to inline the model
> of g_malloc_n in g_malloc (and likewise for the other five functions)
> though it seems like Coverity.  This is implemented in patches 5-6.
>
> On top of these changes, this includes a few more changes to the model
> file:
>
> - patch 1 include a few more simplified memory read/write models, so
>   that Coverity has a model for all functions in the pci_dma_* and
>   dma_memory_* family.  This fixes a few incorrect out of bounds
>   false positive, where Coverity does not realize that only up to
>   LEN bytes are read/written by those functions
>
> - patch 3 removes the model for various allocation functions, which
>   is unnecessary now that we need not (or cannot) detect their
>   being paired with g_free
>
> - patch 4 is a small cleanup that makes the inlined allocation
>   functions smaller.
>
> This series is a sort of FYI; since the only way to debug the model file
> is to upload it to scan.coverity.com, these changes are all already live.
> The last will be as of the next build, but was effective last Thursday
> and worked (I tried disabling it on Friday in something like a bisection,
> but it failed and I have now reverted to Thursday's model).

Thanks for digging through all this mess. I take it that the
Coverity results are now stable and people can now start looking
through them and triaging again ?

-- PMM

Re: [PATCH 4/6] coverity-model: clean up the models for array allocation functions

[Paolo Bonzini]

On 02/08/21 14:36, Peter Maydell wrote:
> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> 
> The real g_malloc_n() returns failure if the multiplication
> would overflow; I guess Coverity currently doesn't have any
> warnings it generates as a result of assuming overflow
> might happen?

I couldn't find any Coverity-specific way to detect overflow, but making 
nmemb a tainted sink could be an interesting way to ensure that 
untrusted data does not end up causing such a failure.

Likewise, we should try making __bufwrite taint the buffer it is writing 
to; there's already a TODO for that but I never followed up on it.

Paolo

Re: [PATCH 0/6] Updates for Coverity modeling file

[Paolo Bonzini]

On 02/08/21 14:46, Peter Maydell wrote:
>> This series is a sort of FYI; since the only way to debug the model file
>> is to upload it to scan.coverity.com, these changes are all already live.
>> The last will be as of the next build, but was effective last Thursday
>> and worked (I tried disabling it on Friday in something like a bisection,
>> but it failed and I have now reverted to Thursday's model).
> Thanks for digging through all this mess. I take it that the
> Coverity results are now stable and people can now start looking
> through them and triaging again ?

Yes, these patches are the final result of the "investigation".

Paolo

Re: [PATCH 0/6] Updates for Coverity modeling file

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

[...]

> This series is a sort of FYI; since the only way to debug the model file
> is to upload it to scan.coverity.com, these changes are all already live.

When I mess with Coverity, I test with my locally installed version
first.  Version skew and lack of the web interface can make that less
than useful sometimes.

This is not criticism of your work flow.

> The last will be as of the next build, but was effective last Thursday
> and worked (I tried disabling it on Friday in something like a bisection,
> but it failed and I have now reverted to Thursday's model).

Re: [PATCH 4/6] coverity-model: clean up the models for array allocation functions

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 02/08/21 14:36, Peter Maydell wrote:
>> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
>> The real g_malloc_n() returns failure if the multiplication
>> would overflow;

Really?  Its documentation:

    This function is similar to g_malloc(), allocating (n_blocks *
    n_block_bytes) bytes, but care is taken to detect possible overflow
    during multiplication.

There's also g_try_malloc_n():

    This function is similar to g_try_malloc(), allocating (n_blocks *
    n_block_bytes) bytes, but care is taken to detect possible overflow
    during multiplication.

I read this as g_malloc_n() can return null only for zero size, and
crashes on error, just like g_malloc_n().  g_try_malloc_n() behaves like
g_try_malloc_n(), i.e. it returns null on failure.

>>                 I guess Coverity currently doesn't have any
>> warnings it generates as a result of assuming overflow
>> might happen?
>
> I couldn't find any Coverity-specific way to detect overflow, but

Does it need one?

Quick peek at Coverity 2012.12 Checker Reference:

    4.188. INTEGER_OVERFLOW
    [...]
    4.188.1. Overview

    Supported Languages: C, C++, Objective-C, Objective-C++

    INTEGER_OVERFLOW finds many cases of integer overflows and
    truncations resulting from arithmetic operations. Some forms of
    integer overflow can cause security vulnerabilities, for example,
    when the overflowed value is used as the argument to an allocation
    function. [...]

    [...]

    Disabled by default: INTEGER_OVERFLOW is disabled by default. To
    enable it, you can use the --enable option to the cov-analyze
    command.

The example that follows shows a memory allocation where the size is
computed like TAINTED * sizeof(MUMBLE), where TAINTED is a "tainted
source", and the allocation's size is a "tainted sink".

Our model for g_malloc_n() uses __coverity_alloc(), which should provide
"tainted sink".

> making nmemb a tainted sink could be an interesting way to ensure that 
> untrusted data does not end up causing such a failure.
>
> Likewise, we should try making __bufwrite taint the buffer it is
> writing to; there's already a TODO for that but I never followed up on
> it.

__bufwrite() tells Coverity that the buf[len] bytes are overwritten with
unspecified data.  I'd expect that to taint the buffer already.

Re: [PATCH 4/6] coverity-model: clean up the models for array allocation functions

[Peter Maydell]

On Wed, 4 Aug 2021 at 09:35, Markus Armbruster <armbru@redhat.com> wrote:
>
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
> > On 02/08/21 14:36, Peter Maydell wrote:
> >> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> >> The real g_malloc_n() returns failure if the multiplication
> >> would overflow;
>
> Really?  Its documentation:
>
>     This function is similar to g_malloc(), allocating (n_blocks *
>     n_block_bytes) bytes, but care is taken to detect possible overflow
>     during multiplication.
>
> There's also g_try_malloc_n():
>
>     This function is similar to g_try_malloc(), allocating (n_blocks *
>     n_block_bytes) bytes, but care is taken to detect possible overflow
>     during multiplication.
>
> I read this as g_malloc_n() can return null only for zero size, and
> crashes on error, just like g_malloc_n().  g_try_malloc_n() behaves like
> g_try_malloc_n(), i.e. it returns null on failure.

Yeah, I misspoke -- I just meant to say "treats multiply overflow
like an allocation failure", not that it returns NULL.

> >>                 I guess Coverity currently doesn't have any
> >> warnings it generates as a result of assuming overflow
> >> might happen?
> >
> > I couldn't find any Coverity-specific way to detect overflow, but
>
> Does it need one?
>
> Quick peek at Coverity 2012.12 Checker Reference:
>
>     4.188. INTEGER_OVERFLOW

> The example that follows shows a memory allocation where the size is
> computed like TAINTED * sizeof(MUMBLE), where TAINTED is a "tainted
> source", and the allocation's size is a "tainted sink".

My point was that we *don't* want Coverity to report INTEGER_OVERFLOW
errors based on the way our g_malloc_n() model is written like this:
  __coverity_alloc__(nmemb * size)
That expression in our model *can* overflow. We know the real g_malloc_n()
cannot overflow, but we don't do anything to tell Coverity that, so it's
possible that (maybe in future) it will report false positives to us
based on analysis that assumes possible overflows in the multiplication.

thanks
-- PMM