[PATCH] memory: initialize 'fv' in MemoryRegionCache to make Coverity happy

[PATCH] memory: initialize 'fv' in MemoryRegionCache to make Coverity happy

[Ilya Maximets]

Coverity scan reports multiple false-positive "defects" for the
following series of actions in virtio.c:

  MemoryRegionCache indirect_desc_cache;
  address_space_cache_init_empty(&indirect_desc_cache);
  address_space_cache_destroy(&indirect_desc_cache);

For some reason it's unable to recognize the dependency between 'mrs.mr'
and 'fv' and insists that '!mrs.mr' check in address_space_cache_destroy
may take a 'false' branch, even though it is explicitly initialized to
NULL in the address_space_cache_init_empty():

  *** CID 1522371:  Memory - illegal accesses  (UNINIT)
  /qemu/hw/virtio/virtio.c: 1627 in virtqueue_split_pop()
  1621         }
  1622
  1623         vq->inuse++;
  1624
  1625         trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
  1626     done:
  >>>     CID 1522371:  Memory - illegal accesses  (UNINIT)
  >>>     Using uninitialized value "indirect_desc_cache.fv" when
  >>>     calling "address_space_cache_destroy".
  1627         address_space_cache_destroy(&indirect_desc_cache);
  1628
  1629         return elem;
  1630
  1631     err_undo_map:
  1632         virtqueue_undo_map_desc(out_num, in_num, iov);

  ** CID 1522370:  Memory - illegal accesses  (UNINIT)

Instead of trying to silence these false positive reports in 4
different places, initializing 'fv' as well, as this doesn't result
in any noticeable performance impact.

Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 include/exec/memory.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index c99842d2fc..1ce80c4e82 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2770,6 +2770,8 @@ int64_t address_space_cache_init(MemoryRegionCache *cache,
 static inline void address_space_cache_init_empty(MemoryRegionCache *cache)
 {
     cache->mrs.mr = NULL;
+    /* There is no real need to initialize fv, but it makes Coverity happy. */
+    cache->fv = NULL;
 }
 
 /**
-- 
2.41.0

Re: [PATCH] memory: initialize 'fv' in MemoryRegionCache to make Coverity happy

[David Hildenbrand]

On 09.10.23 12:43, Ilya Maximets wrote:
> Coverity scan reports multiple false-positive "defects" for the
> following series of actions in virtio.c:
> 
>    MemoryRegionCache indirect_desc_cache;
>    address_space_cache_init_empty(&indirect_desc_cache);
>    address_space_cache_destroy(&indirect_desc_cache);
> 
> For some reason it's unable to recognize the dependency between 'mrs.mr'
> and 'fv' and insists that '!mrs.mr' check in address_space_cache_destroy
> may take a 'false' branch, even though it is explicitly initialized to
> NULL in the address_space_cache_init_empty():
> 
>    *** CID 1522371:  Memory - illegal accesses  (UNINIT)
>    /qemu/hw/virtio/virtio.c: 1627 in virtqueue_split_pop()
>    1621         }
>    1622
>    1623         vq->inuse++;
>    1624
>    1625         trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
>    1626     done:
>    >>>     CID 1522371:  Memory - illegal accesses  (UNINIT)
>    >>>     Using uninitialized value "indirect_desc_cache.fv" when
>    >>>     calling "address_space_cache_destroy".
>    1627         address_space_cache_destroy(&indirect_desc_cache);

Yeah, it doesn't even care about what that function actually does, just 
that it is called with a datastructure that is partially uninitialized.

>    1628
>    1629         return elem;
>    1630
>    1631     err_undo_map:
>    1632         virtqueue_undo_map_desc(out_num, in_num, iov);
> 
>    ** CID 1522370:  Memory - illegal accesses  (UNINIT)
> 
> Instead of trying to silence these false positive reports in 4
> different places, initializing 'fv' as well, as this doesn't result
> in any noticeable performance impact.
> 
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
>   include/exec/memory.h | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index c99842d2fc..1ce80c4e82 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2770,6 +2770,8 @@ int64_t address_space_cache_init(MemoryRegionCache *cache,
>   static inline void address_space_cache_init_empty(MemoryRegionCache *cache)
>   {
>       cache->mrs.mr = NULL;
> +    /* There is no real need to initialize fv, but it makes Coverity happy. */
> +    cache->fv = NULL;
>   }
>   
>   /**

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH] memory: initialize 'fv' in MemoryRegionCache to make Coverity happy

[Stefan Hajnoczi]

On Mon, 9 Oct 2023 at 06:44, Ilya Maximets <i.maximets@ovn.org> wrote:
>
> Coverity scan reports multiple false-positive "defects" for the
> following series of actions in virtio.c:
>
>   MemoryRegionCache indirect_desc_cache;
>   address_space_cache_init_empty(&indirect_desc_cache);
>   address_space_cache_destroy(&indirect_desc_cache);
>
> For some reason it's unable to recognize the dependency between 'mrs.mr'
> and 'fv' and insists that '!mrs.mr' check in address_space_cache_destroy
> may take a 'false' branch, even though it is explicitly initialized to
> NULL in the address_space_cache_init_empty():
>
>   *** CID 1522371:  Memory - illegal accesses  (UNINIT)
>   /qemu/hw/virtio/virtio.c: 1627 in virtqueue_split_pop()
>   1621         }
>   1622
>   1623         vq->inuse++;
>   1624
>   1625         trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
>   1626     done:
>   >>>     CID 1522371:  Memory - illegal accesses  (UNINIT)
>   >>>     Using uninitialized value "indirect_desc_cache.fv" when
>   >>>     calling "address_space_cache_destroy".
>   1627         address_space_cache_destroy(&indirect_desc_cache);
>   1628
>   1629         return elem;
>   1630
>   1631     err_undo_map:
>   1632         virtqueue_undo_map_desc(out_num, in_num, iov);
>
>   ** CID 1522370:  Memory - illegal accesses  (UNINIT)
>
> Instead of trying to silence these false positive reports in 4
> different places, initializing 'fv' as well, as this doesn't result
> in any noticeable performance impact.
>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
>  include/exec/memory.h | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>