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

[Greg Kurz <groug@kaod.org>]

[-- 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 --]