On Fri, 12 Jun 2020 at 15:52, Michael S. Tsirkin <m...@redhat.com> wrote: > > From: Raphael Norwitz <raphael.norw...@nutanix.com> > > When setting vhost-user memory tables, memory region descriptors must be > copied from the vhost_dev struct to the vhost-user message. To avoid > duplicating code in setting the memory tables, we should use a helper to > populate this field. This change adds this helper. > > Signed-off-by: Raphael Norwitz <raphael.norw...@nutanix.com> > Message-Id: <1588533678-23450-2-git-send-email-raphael.norw...@nutanix.com> > Reviewed-by: Michael S. Tsirkin <m...@redhat.com> > Signed-off-by: Michael S. Tsirkin <m...@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
Hi; Coverity reports a problem with this patch (CID 1429804): > --- > hw/virtio/vhost-user.c | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index ec21e8fbe8..2e0552dd74 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -407,6 +407,15 @@ static int vhost_user_set_log_base(struct vhost_dev > *dev, uint64_t base, > return 0; > } > > +static void vhost_user_fill_msg_region(VhostUserMemoryRegion *dst, > + struct vhost_memory_region *src) > +{ > + assert(src != NULL && dst != NULL); > + dst->userspace_addr = src->userspace_addr; > + dst->memory_size = src->memory_size; > + dst->guest_phys_addr = src->guest_phys_addr; This function only initializes 3 of the 4 fields of the VhostUserMemoryRegion struct... > +} > + > static int vhost_user_fill_set_mem_table_msg(struct vhost_user *u, > struct vhost_dev *dev, > VhostUserMsg *msg, > @@ -417,6 +426,7 @@ static int vhost_user_fill_set_mem_table_msg(struct > vhost_user *u, > ram_addr_t offset; > MemoryRegion *mr; > struct vhost_memory_region *reg; > + VhostUserMemoryRegion region_buffer; ...this variable starts uninitialized... > > msg->hdr.request = VHOST_USER_SET_MEM_TABLE; > > @@ -441,12 +451,8 @@ static int vhost_user_fill_set_mem_table_msg(struct > vhost_user *u, > error_report("Failed preparing vhost-user memory table msg"); > return -1; > } > - msg->payload.memory.regions[*fd_num].userspace_addr = > - reg->userspace_addr; > - msg->payload.memory.regions[*fd_num].memory_size = > - reg->memory_size; > - msg->payload.memory.regions[*fd_num].guest_phys_addr = > - reg->guest_phys_addr; > + vhost_user_fill_msg_region(®ion_buffer, reg); > + msg->payload.memory.regions[*fd_num] = region_buffer; ...so this struct copy is copying uninitialized data... > msg->payload.memory.regions[*fd_num].mmap_offset = offset; ...which coverity complains about even though it happens that the following line fills in that field in the target of the struct copy. > fds[(*fd_num)++] = fd; > } else if (track_ramblocks) { Coverity also complains about both of the other places that call this function for similar reasons. My suggested fix: make vhost_user_fill_msg_region() take an extra argument "uint64_t mmap_offset", which it uses to initialize the dst->mmap_offset. Then you can pass in "offset" at this callsite and delete the manual initialization of .mmap_offset; and similarly for the other two callsites. thanks -- PMM