[Qemu-devel] [PATCH] Fix issues affecting Xen 9pfs discovered by Coverity

[Qemu-devel] [PATCH] Fix issues affecting Xen 9pfs discovered by Coverity

[Stefano Stabellini]

Fix two resource leaks on error paths, discovered by Coverity.
Check for errors returned by fcntl, also found by Coverity.

CID:1374836
CID:1374831

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 9c7f41a..f75e728 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);
@@ -378,7 +380,10 @@ 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);
+        if (fcntl(xenevtchn_fd(xen_9pdev->rings[i].evtchndev),
+                  F_SETFD, FD_CLOEXEC) == -1) {
+            goto out;
+        }
         xen_9pdev->rings[i].local_port = xenevtchn_bind_interdomain
                                             (xen_9pdev->rings[i].evtchndev,
                                              xendev->dom,

Re: [Qemu-devel] [PATCH] Fix issues affecting Xen 9pfs discovered by Coverity

[Eric Blake]

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

On 05/08/2017 03:45 PM, Stefano Stabellini wrote:
> Fix two resource leaks on error paths, discovered by Coverity.
> Check for errors returned by fcntl, also found by Coverity.
> 
> CID:1374836
> CID:1374831
> 

> @@ -378,7 +380,10 @@ 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);
> +        if (fcntl(xenevtchn_fd(xen_9pdev->rings[i].evtchndev),
> +                  F_SETFD, FD_CLOEXEC) == -1) {
> +            goto out;

Directly calling fcntl(F_SETFD) without first reading fcntl(F_GETFD) is
(theoretically) incorrect.  Better might be using qemu_set_cloexec()
instead of open-coding something.

-- 
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] Fix issues affecting Xen 9pfs discovered by Coverity

[Stefano Stabellini]

On Mon, 8 May 2017, Eric Blake wrote:
> On 05/08/2017 03:45 PM, Stefano Stabellini wrote:
> > Fix two resource leaks on error paths, discovered by Coverity.
> > Check for errors returned by fcntl, also found by Coverity.
> > 
> > CID:1374836
> > CID:1374831
> > 
> 
> > @@ -378,7 +380,10 @@ 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);
> > +        if (fcntl(xenevtchn_fd(xen_9pdev->rings[i].evtchndev),
> > +                  F_SETFD, FD_CLOEXEC) == -1) {
> > +            goto out;
> 
> Directly calling fcntl(F_SETFD) without first reading fcntl(F_GETFD) is
> (theoretically) incorrect.  Better might be using qemu_set_cloexec()
> instead of open-coding something.

Makes sense but the unchecked return of fcntl, discovered by Coverity,
would remain unfixed by calling qemu_set_cloexec here. I don't think I
am up for fixing all the call sites of qemu_set_cloexec.

I am going to drop this change, and resend this patch was only the other
two fixes, fixing 1374836 only.

Re: [Qemu-devel] [PATCH] Fix issues affecting Xen 9pfs discovered by Coverity

[Stefano Stabellini]

On Mon, 8 May 2017, Stefano Stabellini wrote:
> On Mon, 8 May 2017, Eric Blake wrote:
> > On 05/08/2017 03:45 PM, Stefano Stabellini wrote:
> > > Fix two resource leaks on error paths, discovered by Coverity.
> > > Check for errors returned by fcntl, also found by Coverity.
> > > 
> > > CID:1374836
> > > CID:1374831
> > > 
> > 
> > > @@ -378,7 +380,10 @@ 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);
> > > +        if (fcntl(xenevtchn_fd(xen_9pdev->rings[i].evtchndev),
> > > +                  F_SETFD, FD_CLOEXEC) == -1) {
> > > +            goto out;
> > 
> > Directly calling fcntl(F_SETFD) without first reading fcntl(F_GETFD) is
> > (theoretically) incorrect.  Better might be using qemu_set_cloexec()
> > instead of open-coding something.
> 
> Makes sense but the unchecked return of fcntl, discovered by Coverity,
> would remain unfixed by calling qemu_set_cloexec here. I don't think I
> am up for fixing all the call sites of qemu_set_cloexec.
> 
> I am going to drop this change, and resend this patch was only the other
> two fixes, fixing 1374836 only.

Unless you would be fine with:

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] Fix issues affecting Xen 9pfs discovered by Coverity

[Eric Blake]

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

On 05/08/2017 05:00 PM, Stefano Stabellini wrote:

>>> Directly calling fcntl(F_SETFD) without first reading fcntl(F_GETFD) is
>>> (theoretically) incorrect.  Better might be using qemu_set_cloexec()
>>> instead of open-coding something.
>>
>> Makes sense but the unchecked return of fcntl, discovered by Coverity,
>> would remain unfixed by calling qemu_set_cloexec here. I don't think I
>> am up for fixing all the call sites of qemu_set_cloexec.
>>
>> I am going to drop this change, and resend this patch was only the other
>> two fixes, fixing 1374836 only.
> 
> Unless you would be fine with:
> 
> 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);

Seems reasonable to me, but I don't know if anyone else would object.

Changes semantics if someone ever calls qemu_set_cloexec(-1) (previously
it would ignore the EBADF failures, now it will abort) - such callers
are arguably broken, so that's okay by me.

-- 
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] Fix issues affecting Xen 9pfs discovered by Coverity

[Greg Kurz]

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

On Mon, 8 May 2017 17:05:01 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 05/08/2017 05:00 PM, Stefano Stabellini wrote:
> 
> >>> Directly calling fcntl(F_SETFD) without first reading fcntl(F_GETFD) is
> >>> (theoretically) incorrect.  Better might be using qemu_set_cloexec()
> >>> instead of open-coding something.  
> >>
> >> Makes sense but the unchecked return of fcntl, discovered by Coverity,
> >> would remain unfixed by calling qemu_set_cloexec here. I don't think I
> >> am up for fixing all the call sites of qemu_set_cloexec.
> >>
> >> I am going to drop this change, and resend this patch was only the other
> >> two fixes, fixing 1374836 only.  
> > 
> > Unless you would be fine with:
> > 
> > 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);  
> 
> Seems reasonable to me, but I don't know if anyone else would object.
> 
> Changes semantics if someone ever calls qemu_set_cloexec(-1) (previously
> it would ignore the EBADF failures, now it will abort) - such callers
> are arguably broken, so that's okay by me.
> 

I've checked all current users and they all pass a valid fd to
qemu_set_cloexec(). Also F_SETFD/F_GETFD is required by POSIX
and we cannot get an EINVAL failure either. I guess the change
is ok then.

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