Fix issues affecting Xen 9pfs discovered by Coverity

Commit Message

Stefano Stabellini May 8, 2017, 10 p.m. UTC

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:

Comments

Eric Blake May 8, 2017, 10:05 p.m. UTC | #1

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.

Greg Kurz May 9, 2017, 12:28 p.m. UTC | #2

On Mon, 8 May 2017 17:05:01 -0500
Eric Blake <[e-mail]> 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.

Patch

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);
 }
 
 /*