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

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

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

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).