[Qemu-devel] [PATCH 0/3] vhost-scsi, ivshmem, savevm fixes for Coverity issues

[Qemu-devel] [PATCH 0/3] vhost-scsi, ivshmem, savevm fixes for Coverity issues

[Stefan Hajnoczi]

The following fixes address NULL pointer dereferences and resource leaks
spotted by Coverity.

Stefan Hajnoczi (3):
  vhost-scsi: fix k->set_guest_notifiers() NULL dereference
  ivshmem: add missing error exit(2)
  savevm: avoid leaking popen(3) file pointer

 hw/misc/ivshmem.c    | 1 +
 hw/scsi/vhost-scsi.c | 2 +-
 savevm.c             | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/3] vhost-scsi: fix k->set_guest_notifiers() NULL dereference

[Stefan Hajnoczi]

Coverity picked up a copy-paste bug.  In vhost_scsi_start() we check for
!k->set_guest_notifiers and error out.  The check probably got copied
but instead of erroring we actually use the function pointer!

Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Asias He <asias@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/scsi/vhost-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index d7a1c33..785e93f 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -123,7 +123,7 @@ static void vhost_scsi_stop(VHostSCSI *s)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     int ret = 0;
 
-    if (!k->set_guest_notifiers) {
+    if (k->set_guest_notifiers) {
         ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
         if (ret < 0) {
                 error_report("vhost guest notifier cleanup failed: %d\n", ret);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/3] vhost-scsi: fix k->set_guest_notifiers() NULL dereference

[mdroth]

On Thu, May 30, 2013 at 04:14:44PM +0200, Stefan Hajnoczi wrote:
> Coverity picked up a copy-paste bug.  In vhost_scsi_start() we check for
> !k->set_guest_notifiers and error out.  The check probably got copied
> but instead of erroring we actually use the function pointer!
> 
> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
> Cc: Asias He <asias@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Hi Paolo,

Looking to pick this up for 1.5.1 along with a few other goodies in your
scsi branch:

iscsi: reorganize iscsi_readcapacity_sync
iscsi: simplify freeing of tasks
scsi-disk: scsi-block device for scsi pass-through should not be remo…
scsi-generic: check the return value of bdrv_aio_ioctl in execute_com…
scsi-generic: fix sign extension of READ CAPACITY(10) data
scsi: reset cdrom tray statuses on scsi_disk_reset

Freeze for 1.5.1 is planned for June 19. Willing to pluck from
maintainer branches for the more important ones but would prefer
upstream if you can send a PULL for these.

> ---
>  hw/scsi/vhost-scsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index d7a1c33..785e93f 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -123,7 +123,7 @@ static void vhost_scsi_stop(VHostSCSI *s)
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      int ret = 0;
> 
> -    if (!k->set_guest_notifiers) {
> +    if (k->set_guest_notifiers) {
>          ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
>          if (ret < 0) {
>                  error_report("vhost guest notifier cleanup failed: %d\n", ret);
> -- 
> 1.8.1.4
> 
> 

Re: [Qemu-devel] [PATCH 1/3] vhost-scsi: fix k->set_guest_notifiers() NULL dereference

[Paolo Bonzini]

Il 11/06/2013 18:58, mdroth ha scritto:
> iscsi: reorganize iscsi_readcapacity_sync
> iscsi: simplify freeing of tasks
> scsi-disk: scsi-block device for scsi pass-through should not be remo…
> scsi-generic: check the return value of bdrv_aio_ioctl in execute_com…
> scsi-generic: fix sign extension of READ CAPACITY(10) data
> scsi: reset cdrom tray statuses on scsi_disk_reset
> 
> Freeze for 1.5.1 is planned for June 19. Willing to pluck from
> maintainer branches for the more important ones but would prefer
> upstream if you can send a PULL for these.

Yes, I'll be back from holiday on Monday and will send pull requests then.

Paolo

[Qemu-devel] [PATCH 2/3] ivshmem: add missing error exit(2)

[Stefan Hajnoczi]

If the user fails to specify 'chardev' or 'shm' then we cannot continue.
Exit right away so that we don't invoke shm_open(3) with a NULL pointer.

It would be nice to replace exit(1) with error returns in the PCI device
.init() function, but leave that for another patch since exit(1) is
currently used elsewhere.

Spotted by Coverity.

Cc: Cam Macdonell <cam@cs.ualberta.ca>
Cc: qemu-stable@nongnu.org
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/misc/ivshmem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index a19a6d6..5658f73 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -735,6 +735,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
 
         if (s->shmobj == NULL) {
             fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n");
+            exit(1);
         }
 
         IVSHMEM_DPRINTF("using shm_open (shm object = %s)\n", s->shmobj);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/3] ivshmem: add missing error exit(2)

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> If the user fails to specify 'chardev' or 'shm' then we cannot continue.
> Exit right away so that we don't invoke shm_open(3) with a NULL pointer.
>
> It would be nice to replace exit(1) with error returns in the PCI device
> .init() function, but leave that for another patch since exit(1) is
> currently used elsewhere.
>
> Spotted by Coverity.
>
> Cc: Cam Macdonell <cam@cs.ualberta.ca>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/misc/ivshmem.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index a19a6d6..5658f73 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -735,6 +735,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
>  
>          if (s->shmobj == NULL) {
>              fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n");
> +            exit(1);
>          }
>  
>          IVSHMEM_DPRINTF("using shm_open (shm object = %s)\n", s->shmobj);

Botching a hotplug of this device kills the VM.  Nasty trap, but no
worse than before.

[Qemu-devel] [PATCH 3/3] savevm: avoid leaking popen(3) file pointer

[Stefan Hajnoczi]

I'm not sure why we check the mode only after invoking popen(3) but we
need to close the file pointer.

Spotted by Coverity.

Cc: Juan Quintela <quintela@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 savevm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/savevm.c b/savevm.c
index 31dcce9..75cc72e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -329,6 +329,7 @@ QEMUFile *qemu_popen_cmd(const char *command, const char *mode)
 
     if (mode == NULL || (mode[0] != 'r' && mode[0] != 'w') || mode[1] != 0) {
         fprintf(stderr, "qemu_popen: Argument validity check failed\n");
+        fclose(stdio_file);
         return NULL;
     }
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 3/3] savevm: avoid leaking popen(3) file pointer

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

On 05/30/2013 08:14 AM, Stefan Hajnoczi wrote:
> I'm not sure why we check the mode only after invoking popen(3) but we
> need to close the file pointer.
> 
> Spotted by Coverity.
> 
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  savevm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/savevm.c b/savevm.c
> index 31dcce9..75cc72e 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -329,6 +329,7 @@ QEMUFile *qemu_popen_cmd(const char *command, const char *mode)
>  
>      if (mode == NULL || (mode[0] != 'r' && mode[0] != 'w') || mode[1] != 0) {
>          fprintf(stderr, "qemu_popen: Argument validity check failed\n");
> +        fclose(stdio_file);

You MUST use pclose() (not fclose) on any FILE obtained by popen(), to
avoid resource leaks.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 3/3] savevm: avoid leaking popen(3) file pointer

[Stefan Hajnoczi]

On Thu, May 30, 2013 at 4:25 PM, Eric Blake <eblake@redhat.com> wrote:
> On 05/30/2013 08:14 AM, Stefan Hajnoczi wrote:
>> I'm not sure why we check the mode only after invoking popen(3) but we
>> need to close the file pointer.
>>
>> Spotted by Coverity.
>>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  savevm.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/savevm.c b/savevm.c
>> index 31dcce9..75cc72e 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -329,6 +329,7 @@ QEMUFile *qemu_popen_cmd(const char *command, const char *mode)
>>
>>      if (mode == NULL || (mode[0] != 'r' && mode[0] != 'w') || mode[1] != 0) {
>>          fprintf(stderr, "qemu_popen: Argument validity check failed\n");
>> +        fclose(stdio_file);
>
> You MUST use pclose() (not fclose) on any FILE obtained by popen(), to
> avoid resource leaks.

Thanks, I didn't know that.  Should have checked the popen(3) man page.

Will fix.

Stefan

Re: [Qemu-devel] [Qemu-stable] [PATCH 3/3] savevm: avoid leaking popen(3) file pointer

[mdroth]

On Thu, May 30, 2013 at 04:14:46PM +0200, Stefan Hajnoczi wrote:
> I'm not sure why we check the mode only after invoking popen(3) but we
> need to close the file pointer.
> 
> Spotted by Coverity.
> 
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Ping, looking to get this in for 1.5.1

> ---
>  savevm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/savevm.c b/savevm.c
> index 31dcce9..75cc72e 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -329,6 +329,7 @@ QEMUFile *qemu_popen_cmd(const char *command, const char *mode)
> 
>      if (mode == NULL || (mode[0] != 'r' && mode[0] != 'w') || mode[1] != 0) {
>          fprintf(stderr, "qemu_popen: Argument validity check failed\n");
> +        fclose(stdio_file);
>          return NULL;
>      }
> 
> -- 
> 1.8.1.4
> 
> 

Re: [Qemu-devel] [Qemu-stable] [PATCH 3/3] savevm: avoid leaking popen(3) file pointer

[Stefan Hajnoczi]

On Tue, Jun 11, 2013 at 05:59:31PM -0500, mdroth wrote:
> On Thu, May 30, 2013 at 04:14:46PM +0200, Stefan Hajnoczi wrote:
> > I'm not sure why we check the mode only after invoking popen(3) but we
> > need to close the file pointer.
> > 
> > Spotted by Coverity.
> > 
> > Cc: Juan Quintela <quintela@redhat.com>
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Ping, looking to get this in for 1.5.1
> 
> > ---
> >  savevm.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/savevm.c b/savevm.c
> > index 31dcce9..75cc72e 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -329,6 +329,7 @@ QEMUFile *qemu_popen_cmd(const char *command, const char *mode)
> > 
> >      if (mode == NULL || (mode[0] != 'r' && mode[0] != 'w') || mode[1] != 0) {
> >          fprintf(stderr, "qemu_popen: Argument validity check failed\n");
> > +        fclose(stdio_file);
> >          return NULL;
> >      }

The v2 I sent had Patch 1 picked up by Paolo and Patch 2 is not yet
merged by anyone.

Stefan