| Message ID | 20240930081458.1926382-20-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | -Werror=maybe-uninitialized fixes | expand |
On Mon, Sep 30, 2024 at 10:17 AM <marcandre.lureau@redhat.com> wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > vhost_svq_get_buf() may return a VirtQueueElement that should be freed. > > It's unclear to me if the vhost_svq_get_buf() call should always return NULL. > Continuing conversation of v2, Yes there are situations where vhost_svq_get_buf can return a valid buffer here and we could leak memory, so this fixes a bug. So, Reviewed-by: Eugenio Pérez <eperezma@redhat.com> Thanks! > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/virtio/vhost-shadow-virtqueue.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > index 3b2beaea24..37aca8b431 100644 > --- a/hw/virtio/vhost-shadow-virtqueue.c > +++ b/hw/virtio/vhost-shadow-virtqueue.c > @@ -414,6 +414,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq, > return i; > } > > +G_GNUC_WARN_UNUSED_RESULT > static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq, > uint32_t *len) > { > @@ -528,6 +529,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num) > size_t len = 0; > > while (num--) { > + g_autofree VirtQueueElement *elem = NULL; > int64_t start_us = g_get_monotonic_time(); > uint32_t r = 0; > > @@ -541,7 +543,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num) > } > } while (true); > > - vhost_svq_get_buf(svq, &r); > + elem = vhost_svq_get_buf(svq, &r); > len += r; > } > > -- > 2.45.2.827.g557ae147e6 >
On Mon, Sep 30, 2024 at 1:02 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Mon, Sep 30, 2024 at 10:17 AM <marcandre.lureau@redhat.com> wrote: > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > vhost_svq_get_buf() may return a VirtQueueElement that should be freed. > > > > It's unclear to me if the vhost_svq_get_buf() call should always return NULL. > > > > Continuing conversation of v2, > > Yes there are situations where vhost_svq_get_buf can return a valid > buffer here and we could leak memory, so this fixes a bug. > > So, > > Reviewed-by: Eugenio Pérez <eperezma@redhat.com> > > Thanks! > (I hit "send" too early) Wwe could use a better patch subject though. Even "Freeing leaked memory from vhost_svq_get_buf in vhost_svq_poll" would work better for me. What do you think? Thanks! > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > hw/virtio/vhost-shadow-virtqueue.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > > index 3b2beaea24..37aca8b431 100644 > > --- a/hw/virtio/vhost-shadow-virtqueue.c > > +++ b/hw/virtio/vhost-shadow-virtqueue.c > > @@ -414,6 +414,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq, > > return i; > > } > > > > +G_GNUC_WARN_UNUSED_RESULT > > static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq, > > uint32_t *len) > > { > > @@ -528,6 +529,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num) > > size_t len = 0; > > > > while (num--) { > > + g_autofree VirtQueueElement *elem = NULL; > > int64_t start_us = g_get_monotonic_time(); > > uint32_t r = 0; > > > > @@ -541,7 +543,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num) > > } > > } while (true); > > > > - vhost_svq_get_buf(svq, &r); > > + elem = vhost_svq_get_buf(svq, &r); > > len += r; > > } > > > > -- > > 2.45.2.827.g557ae147e6 > >
On Mon, Sep 30, 2024 at 3:05 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > On Mon, Sep 30, 2024 at 1:02 PM Eugenio Perez Martin > <eperezma@redhat.com> wrote: > > > > On Mon, Sep 30, 2024 at 10:17 AM <marcandre.lureau@redhat.com> wrote: > > > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > > vhost_svq_get_buf() may return a VirtQueueElement that should be freed. > > > > > > It's unclear to me if the vhost_svq_get_buf() call should always > return NULL. > > > > > > > Continuing conversation of v2, > > > > Yes there are situations where vhost_svq_get_buf can return a valid > > buffer here and we could leak memory, so this fixes a bug. > > > > So, > > > > Reviewed-by: Eugenio Pérez <eperezma@redhat.com> > > > > Thanks! > > > > (I hit "send" too early) > > Wwe could use a better patch subject though. Even "Freeing leaked > memory from vhost_svq_get_buf in vhost_svq_poll" would work better for > me. What do you think? > > Thanks! > ok thanks! > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > --- > > > hw/virtio/vhost-shadow-virtqueue.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c > b/hw/virtio/vhost-shadow-virtqueue.c > > > index 3b2beaea24..37aca8b431 100644 > > > --- a/hw/virtio/vhost-shadow-virtqueue.c > > > +++ b/hw/virtio/vhost-shadow-virtqueue.c > > > @@ -414,6 +414,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const > VhostShadowVirtqueue *svq, > > > return i; > > > } > > > > > > +G_GNUC_WARN_UNUSED_RESULT > > > static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq, > > > uint32_t *len) > > > { > > > @@ -528,6 +529,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, > size_t num) > > > size_t len = 0; > > > > > > while (num--) { > > > + g_autofree VirtQueueElement *elem = NULL; > > > int64_t start_us = g_get_monotonic_time(); > > > uint32_t r = 0; > > > > > > @@ -541,7 +543,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, > size_t num) > > > } > > > } while (true); > > > > > > - vhost_svq_get_buf(svq, &r); > > > + elem = vhost_svq_get_buf(svq, &r); > > > len += r; > > > } > > > > > > -- > > > 2.45.2.827.g557ae147e6 > > > > > >
diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c index 3b2beaea24..37aca8b431 100644 --- a/hw/virtio/vhost-shadow-virtqueue.c +++ b/hw/virtio/vhost-shadow-virtqueue.c @@ -414,6 +414,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq, return i; } +G_GNUC_WARN_UNUSED_RESULT static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq, uint32_t *len) { @@ -528,6 +529,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num) size_t len = 0; while (num--) { + g_autofree VirtQueueElement *elem = NULL; int64_t start_us = g_get_monotonic_time(); uint32_t r = 0; @@ -541,7 +543,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num) } } while (true); - vhost_svq_get_buf(svq, &r); + elem = vhost_svq_get_buf(svq, &r); len += r; }