[Qemu-devel] [PATCH v2 1/3] xen/9pfs: fix two resource leaks on error paths, discovered by Coverity

[Qemu-devel] [PATCH v2 1/3] xen/9pfs: fix two resource leaks on error paths, discovered by Coverity

[Stefano Stabellini]

CID: 1374836

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
CC: anthony.perard@citrix.com
CC: groug@kaod.org
CC: aneesh.kumar@linux.vnet.ibm.com
---
 hw/9pfs/xen-9p-backend.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 9c7f41a..a1fdede 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -332,12 +332,14 @@ static int xen_9pfs_connect(struct XenDevice *xendev)
         str = g_strdup_printf("ring-ref%u", i);
         if (xenstore_read_fe_int(&xen_9pdev->xendev, str,
                                  &xen_9pdev->rings[i].ref) == -1) {
+            g_free(str);
             goto out;
         }
         g_free(str);
         str = g_strdup_printf("event-channel-%u", i);
         if (xenstore_read_fe_int(&xen_9pdev->xendev, str,
                                  &xen_9pdev->rings[i].evtchn) == -1) {
+            g_free(str);
             goto out;
         }
         g_free(str);
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/3] Check the return value of fcntl in qemu_set_cloexec

[Stefano Stabellini]

Assert that the return value is not an error. This issue was found by
Coverity.

CID: 1374831

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
CC: groug@kaod.org
CC: pbonzini@redhat.com
CC: Eric Blake <eblake@redhat.com>
---
 util/oslib-posix.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 4d9189e..16894ad 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -182,7 +182,9 @@ void qemu_set_cloexec(int fd)
 {
     int f;
     f = fcntl(fd, F_GETFD);
-    fcntl(fd, F_SETFD, f | FD_CLOEXEC);
+    assert(f != -1);
+    f = fcntl(fd, F_SETFD, f | FD_CLOEXEC);
+    assert(f != -1);
 }
 
 /*
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 2/3] Check the return value of fcntl in qemu_set_cloexec

[Eric Blake]

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

On 05/09/2017 02:04 PM, Stefano Stabellini wrote:
> Assert that the return value is not an error. This issue was found by
> Coverity.
> 
> CID: 1374831
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> CC: groug@kaod.org
> CC: pbonzini@redhat.com
> CC: Eric Blake <eblake@redhat.com>
> ---
>  util/oslib-posix.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 2/3] Check the return value of fcntl in qemu_set_cloexec

[Greg Kurz]

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

On Tue,  9 May 2017 12:04:52 -0700
Stefano Stabellini <sstabellini@kernel.org> wrote:

> Assert that the return value is not an error. This issue was found by
> Coverity.
> 
> CID: 1374831
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> CC: groug@kaod.org
> CC: pbonzini@redhat.com
> CC: Eric Blake <eblake@redhat.com>
> ---
>  util/oslib-posix.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Reviewed-by: Greg Kurz <groug@kaod.org>

> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 4d9189e..16894ad 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -182,7 +182,9 @@ void qemu_set_cloexec(int fd)
>  {
>      int f;
>      f = fcntl(fd, F_GETFD);
> -    fcntl(fd, F_SETFD, f | FD_CLOEXEC);
> +    assert(f != -1);
> +    f = fcntl(fd, F_SETFD, f | FD_CLOEXEC);
> +    assert(f != -1);
>  }
>  
>  /*

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

Re: [Qemu-devel] [PATCH v2 2/3] Check the return value of fcntl in qemu_set_cloexec

[Paolo Bonzini]

On 09/05/2017 21:04, Stefano Stabellini wrote:
> Assert that the return value is not an error. This issue was found by
> Coverity.
> 
> CID: 1374831
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> CC: groug@kaod.org
> CC: pbonzini@redhat.com
> CC: Eric Blake <eblake@redhat.com>

Queued, thanks.

Paolo

> ---
>  util/oslib-posix.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 4d9189e..16894ad 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -182,7 +182,9 @@ void qemu_set_cloexec(int fd)
>  {
>      int f;
>      f = fcntl(fd, F_GETFD);
> -    fcntl(fd, F_SETFD, f | FD_CLOEXEC);
> +    assert(f != -1);
> +    f = fcntl(fd, F_SETFD, f | FD_CLOEXEC);
> +    assert(f != -1);
>  }
>  
>  /*
> 

Re: [Qemu-devel] [PATCH v2 2/3] Check the return value of fcntl in qemu_set_cloexec

[Stefano Stabellini]

On Thu, 11 May 2017, Paolo Bonzini wrote:
> On 09/05/2017 21:04, Stefano Stabellini wrote:
> > Assert that the return value is not an error. This issue was found by
> > Coverity.
> > 
> > CID: 1374831
> > 
> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > CC: groug@kaod.org
> > CC: pbonzini@redhat.com
> > CC: Eric Blake <eblake@redhat.com>
> 
> Queued, thanks.

I am about to send a pull request with the rest of the series, but I'll
leave this one to you.

Cheers,

Stefano


> > ---
> >  util/oslib-posix.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > index 4d9189e..16894ad 100644
> > --- a/util/oslib-posix.c
> > +++ b/util/oslib-posix.c
> > @@ -182,7 +182,9 @@ void qemu_set_cloexec(int fd)
> >  {
> >      int f;
> >      f = fcntl(fd, F_GETFD);
> > -    fcntl(fd, F_SETFD, f | FD_CLOEXEC);
> > +    assert(f != -1);
> > +    f = fcntl(fd, F_SETFD, f | FD_CLOEXEC);
> > +    assert(f != -1);
> >  }
> >  
> >  /*
> > 
> 

[Qemu-devel] [PATCH v2 3/3] xen: call qemu_set_cloexec instead of fcntl

[Stefano Stabellini]

Use the common utility function, which contains checks on return values,
instead of manually calling fcntl.

CID: 1374831

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
CC: anthony.perard@citrix.com
CC: groug@kaod.org
CC: aneesh.kumar@linux.vnet.ibm.com
CC: Eric Blake <eblake@redhat.com>
---
 hw/9pfs/xen-9p-backend.c | 2 +-
 hw/xen/xen_backend.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index a1fdede..5df97c9 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -380,7 +380,7 @@ static int xen_9pfs_connect(struct XenDevice *xendev)
         if (xen_9pdev->rings[i].evtchndev == NULL) {
             goto out;
         }
-        fcntl(xenevtchn_fd(xen_9pdev->rings[i].evtchndev), F_SETFD, FD_CLOEXEC);
+        qemu_set_cloexec(xenevtchn_fd(xen_9pdev->rings[i].evtchndev));
         xen_9pdev->rings[i].local_port = xenevtchn_bind_interdomain
                                             (xen_9pdev->rings[i].evtchndev,
                                              xendev->dom,
diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index c85f163..2cac47d 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -147,7 +147,7 @@ static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
         qdev_unplug(DEVICE(xendev), NULL);
         return NULL;
     }
-    fcntl(xenevtchn_fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC);
+    qemu_set_cloexec(xenevtchn_fd(xendev->evtchndev));
 
     if (ops->flags & DEVOPS_FLAG_NEED_GNTDEV) {
         xendev->gnttabdev = xengnttab_open(NULL, 0);
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 3/3] xen: call qemu_set_cloexec instead of fcntl

[Eric Blake]

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

On 05/09/2017 02:04 PM, Stefano Stabellini wrote:
> Use the common utility function, which contains checks on return values,
> instead of manually calling fcntl.
> 
> CID: 1374831
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> CC: anthony.perard@citrix.com
> CC: groug@kaod.org
> CC: aneesh.kumar@linux.vnet.ibm.com
> CC: Eric Blake <eblake@redhat.com>
> ---
>  hw/9pfs/xen-9p-backend.c | 2 +-
>  hw/xen/xen_backend.c     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 3/3] xen: call qemu_set_cloexec instead of fcntl

[Greg Kurz]

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

On Tue,  9 May 2017 12:04:53 -0700
Stefano Stabellini <sstabellini@kernel.org> wrote:

> Use the common utility function, which contains checks on return values,

... and first calls F_GETFD as recommended by POSIX.1-2001.

http://pubs.opengroup.org/onlinepubs/009695399/functions/fcntl.html

"The arg values to F_GETFD, F_SETFD, F_GETFL, and F_SETFL all represent
flag values to allow for future growth. Applications using these functions
should do a read-modify-write operation on them, rather than assuming that
only the values defined by this volume of IEEE Std 1003.1-2001 are valid.
It is a common error to forget this, particularly in the case of F_SETFD."

> instead of manually calling fcntl.
> 
> CID: 1374831
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> CC: anthony.perard@citrix.com
> CC: groug@kaod.org
> CC: aneesh.kumar@linux.vnet.ibm.com
> CC: Eric Blake <eblake@redhat.com>
> ---
>  hw/9pfs/xen-9p-backend.c | 2 +-
>  hw/xen/xen_backend.c     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 

Anyway,

Reviewed-by: Greg Kurz <groug@kaod.org>

> diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> index a1fdede..5df97c9 100644
> --- a/hw/9pfs/xen-9p-backend.c
> +++ b/hw/9pfs/xen-9p-backend.c
> @@ -380,7 +380,7 @@ static int xen_9pfs_connect(struct XenDevice *xendev)
>          if (xen_9pdev->rings[i].evtchndev == NULL) {
>              goto out;
>          }
> -        fcntl(xenevtchn_fd(xen_9pdev->rings[i].evtchndev), F_SETFD,
> FD_CLOEXEC);
> +        qemu_set_cloexec(xenevtchn_fd(xen_9pdev->rings[i].evtchndev));
>          xen_9pdev->rings[i].local_port = xenevtchn_bind_interdomain
>                                              (xen_9pdev->rings[i].evtchndev,
>                                               xendev->dom,
> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
> index c85f163..2cac47d 100644
> --- a/hw/xen/xen_backend.c
> +++ b/hw/xen/xen_backend.c
> @@ -147,7 +147,7 @@ static struct XenDevice *xen_be_get_xendev(const char
> *type, int dom, int dev, qdev_unplug(DEVICE(xendev), NULL);
>          return NULL;
>      }
> -    fcntl(xenevtchn_fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC);
> +    qemu_set_cloexec(xenevtchn_fd(xendev->evtchndev));
>  
>      if (ops->flags & DEVOPS_FLAG_NEED_GNTDEV) {
>          xendev->gnttabdev = xengnttab_open(NULL, 0);


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

Re: [Qemu-devel] [PATCH v2 1/3] xen/9pfs: fix two resource leaks on error paths, discovered by Coverity

[Eric Blake]

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

On 05/09/2017 02:04 PM, Stefano Stabellini wrote:
> CID: 1374836
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> CC: anthony.perard@citrix.com
> CC: groug@kaod.org
> CC: aneesh.kumar@linux.vnet.ibm.com
> ---
>  hw/9pfs/xen-9p-backend.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 1/3] xen/9pfs: fix two resource leaks on error paths, discovered by Coverity

[Eric Blake]

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

On 05/09/2017 02:20 PM, Eric Blake wrote:
> On 05/09/2017 02:04 PM, Stefano Stabellini wrote:
>> CID: 1374836
>>
>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>> CC: anthony.perard@citrix.com
>> CC: groug@kaod.org
>> CC: aneesh.kumar@linux.vnet.ibm.com
>> ---
>>  hw/9pfs/xen-9p-backend.c | 2 ++
>>  1 file changed, 2 insertions(+)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

By the way, you forgot to send a 0/3 cover letter (at least, there was
no In-Reply-To: header in your 1/3 mail).  That makes it harder to
automate handling of your series; I was going to reply to the series as
a whole.

While it is inconvenient for human readers, it is even worse for some of
the automated patch tooling we have that expects cover letters for any
multi-patch series.  More patch submission tips at
http://wiki.qemu.org/Contribute/SubmitAPatch include how to use 'git
config' to automate the creation of a cover letter.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 1/3] xen/9pfs: fix two resource leaks on error paths, discovered by Coverity

[Stefano Stabellini]

On Tue, 9 May 2017, Eric Blake wrote:
> On 05/09/2017 02:20 PM, Eric Blake wrote:
> > On 05/09/2017 02:04 PM, Stefano Stabellini wrote:
> >> CID: 1374836
> >>
> >> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> >> CC: anthony.perard@citrix.com
> >> CC: groug@kaod.org
> >> CC: aneesh.kumar@linux.vnet.ibm.com
> >> ---
> >>  hw/9pfs/xen-9p-backend.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> By the way, you forgot to send a 0/3 cover letter (at least, there was
> no In-Reply-To: header in your 1/3 mail).  That makes it harder to
> automate handling of your series; I was going to reply to the series as
> a whole.
> 
> While it is inconvenient for human readers, it is even worse for some of
> the automated patch tooling we have that expects cover letters for any
> multi-patch series.  More patch submission tips at
> http://wiki.qemu.org/Contribute/SubmitAPatch include how to use 'git
> config' to automate the creation of a cover letter.

Sorry about that, and thanks for the review.

Re: [Qemu-devel] [PATCH v2 1/3] xen/9pfs: fix two resource leaks on error paths, discovered by Coverity

[Greg Kurz]

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

On Tue,  9 May 2017 12:04:51 -0700
Stefano Stabellini <sstabellini@kernel.org> wrote:

> CID: 1374836
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> CC: anthony.perard@citrix.com
> CC: groug@kaod.org
> CC: aneesh.kumar@linux.vnet.ibm.com
> ---
>  hw/9pfs/xen-9p-backend.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
> index 9c7f41a..a1fdede 100644
> --- a/hw/9pfs/xen-9p-backend.c
> +++ b/hw/9pfs/xen-9p-backend.c
> @@ -332,12 +332,14 @@ static int xen_9pfs_connect(struct XenDevice *xendev)
>          str = g_strdup_printf("ring-ref%u", i);
>          if (xenstore_read_fe_int(&xen_9pdev->xendev, str,
>                                   &xen_9pdev->rings[i].ref) == -1) {
> +            g_free(str);
>              goto out;
>          }
>          g_free(str);

I would rather do something like:

    int ret;

    [...]

        str = g_strdup_printf("ring-ref%u", i);
        ret = xenstore_read_fe_int(&xen_9pdev->xendev, str,
                                   &xen_9pdev->rings[i].ref);
        g_free(str);
        if (ret ==  -1) {
            goto out;
        }

but this is a matter of taste and you own this code so:

Reviewed-by: Greg Kurz <groug@kaod.org>

>          str = g_strdup_printf("event-channel-%u", i);
>          if (xenstore_read_fe_int(&xen_9pdev->xendev, str,
>                                   &xen_9pdev->rings[i].evtchn) == -1) {
> +            g_free(str);
>              goto out;
>          }
>          g_free(str);


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