| Message ID | 1340390174-7493-8-git-send-email-coreyb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 06/22/2012 12:36 PM, Corey Bryant wrote: > This patch adds support to qemu_open to dup(fd) a pre-opened file > descriptor if the filename is of the format /dev/fd/X. > > This can be used when QEMU is restricted from opening files, and > the management application opens files on QEMU's behalf. > > If the fd was passed to the monitor with the pass-fd command, it > must be explicitly closed with the 'closefd' command when it is > no longer required, in order to prevent fd leaks. > > +static int qemu_dup(int fd, int flags) > +{ > + int ret; > + int serrno; > + > + if (flags & O_CLOEXEC) { > + ret = fcntl(fd, F_DUPFD_CLOEXEC, 0); F_DUPFD_CLOEXEC is required by POSIX, but not implemented on all platforms yet. Do you need to be checking with #ifdef F_DUPFD_CLOEXEC to avoid compilation failure? > + if (ret == -1 && errno == EINVAL) { > + ret = dup(fd); > + if (ret == -1) { > + goto fail; > + } > + if (fcntl_setfl(ret, O_CLOEXEC, (flags & O_CLOEXEC) ? 1 : 0) < 0) { Broken. O_CLOEXEC _only_ affects open(); to change it on an existing fd, you have to use fcntl(F_GETFD/F_SETFD) (not F_GETFL/F_SETFL). > + > + if ((fcntl_setfl(ret, O_APPEND, (flags & O_APPEND) ? 1 : 0) < 0) || > + (fcntl_setfl(ret, O_ASYNC, (flags & O_ASYNC) ? 1 : 0) < 0) || > + (fcntl_setfl(ret, O_DIRECT, (flags & O_DIRECT) ? 1 : 0) < 0) || > + (fcntl_setfl(ret, O_LARGEFILE, (flags & O_LARGEFILE) ? 1 : 0) < 0) || Pointless. O_LARGEFILE should _always_ be set, since we are compiling for 64-bit off_t always. > + (fcntl_setfl(ret, O_NDELAY, (flags & O_NDELAY) ? 1 : 0) < 0) || > + (fcntl_setfl(ret, O_NOATIME, (flags & O_NOATIME) ? 1 : 0) < 0) || > + (fcntl_setfl(ret, O_NOCTTY, (flags & O_NOCTTY) ? 1 : 0) < 0) || > + (fcntl_setfl(ret, O_NONBLOCK, (flags & O_NONBLOCK) ? 1 : 0) < 0) || > + (fcntl_setfl(ret, O_SYNC, (flags & O_SYNC) ? 1 : 0) < 0)) { Yuck. That's a lot of syscalls (1 per fcntl_setfl() if they are already set correctly, and 2 per fcntl_setfl() call if we are toggling each one). It might be better to combine this into at most 2 fcntl() calls, instead of a long sequence. > + /* Get the existing fd's flags */ > + eflags = fcntl(fd, F_GETFL); > + if (eflags == -1) { > + return -1; > + } > + > + if (((flags & O_RDWR) != (eflags & O_RDWR)) || > + ((flags & O_RDONLY) != (eflags & O_RDONLY)) || > + ((flags & O_WRONLY) != (eflags & O_WRONLY))) { Broken. O_RDWR, O_RDONLY, and O_WRONLY are NOT bitmasks, but are values in the range of O_ACCMODE. In particular, O_RDONLY==0 on some platforms (Linux), and ==1 on others (Hurd), and although POSIX recommends that O_RDWR==(O_RDONLY|O_WRONLY) for any new systems, no one has really done that except Hurd. A correct way to write this is: switch (flags & O_ACCMODE) { case O_RDWR: if ((eflags & O_ACCMODE) != O_RDWR) { goto error; break; case O_RDONLY: if ((eflags & O_ACCMODE) != O_RDONLY) { goto error; break; case O_RDONLY: if ((eflags & O_ACCMODE) != O_RDONLY) { goto error; break; default: goto error: } [Technically, POSIX also requires O_ACCMODE to include O_SEARCH and O_EXEC, although those two constants might be the same value; but right now Linux has not yet implemented that bit; but unless qemu ever gains the need to open executable binaries with O_EXEC or directories with O_SEARCH, we probably don't have to worry about that aspect of O_ACCMODE here.] > + errno = EACCES; > + return -1; > + } > + > + if (fcntl_setfl(fd, O_CLOEXEC, 1) < 0) { Again, broken. Besides, why are you attempting it both here and in qemu_dup()? Shouldn't once be enough? > + return -1; > + } > + > + return qemu_dup(fd, flags);
On 06/22/2012 03:58 PM, Eric Blake wrote: > On 06/22/2012 12:36 PM, Corey Bryant wrote: >> This patch adds support to qemu_open to dup(fd) a pre-opened file >> descriptor if the filename is of the format /dev/fd/X. >> >> This can be used when QEMU is restricted from opening files, and >> the management application opens files on QEMU's behalf. >> >> If the fd was passed to the monitor with the pass-fd command, it >> must be explicitly closed with the 'closefd' command when it is >> no longer required, in order to prevent fd leaks. >> > >> +static int qemu_dup(int fd, int flags) >> +{ >> + int ret; >> + int serrno; >> + >> + if (flags & O_CLOEXEC) { >> + ret = fcntl(fd, F_DUPFD_CLOEXEC, 0); > > F_DUPFD_CLOEXEC is required by POSIX, but not implemented on all > platforms yet. Do you need to be checking with #ifdef F_DUPFD_CLOEXEC > to avoid compilation failure? > Yes it sounds like this needs to be done. I'll fix this in v5 (as discussed in the previous email). >> + if (ret == -1 && errno == EINVAL) { >> + ret = dup(fd); >> + if (ret == -1) { >> + goto fail; >> + } >> + if (fcntl_setfl(ret, O_CLOEXEC, (flags & O_CLOEXEC) ? 1 : 0) < 0) { > > Broken. O_CLOEXEC _only_ affects open(); to change it on an existing > fd, you have to use fcntl(F_GETFD/F_SETFD) (not F_GETFL/F_SETFL). > > I'll fix this in v5. >> + >> + if ((fcntl_setfl(ret, O_APPEND, (flags & O_APPEND) ? 1 : 0) < 0) || >> + (fcntl_setfl(ret, O_ASYNC, (flags & O_ASYNC) ? 1 : 0) < 0) || >> + (fcntl_setfl(ret, O_DIRECT, (flags & O_DIRECT) ? 1 : 0) < 0) || >> + (fcntl_setfl(ret, O_LARGEFILE, (flags & O_LARGEFILE) ? 1 : 0) < 0) || > > Pointless. O_LARGEFILE should _always_ be set, since we are compiling > for 64-bit off_t always. > I'll remove this in v5. >> + (fcntl_setfl(ret, O_NDELAY, (flags & O_NDELAY) ? 1 : 0) < 0) || >> + (fcntl_setfl(ret, O_NOATIME, (flags & O_NOATIME) ? 1 : 0) < 0) || >> + (fcntl_setfl(ret, O_NOCTTY, (flags & O_NOCTTY) ? 1 : 0) < 0) || >> + (fcntl_setfl(ret, O_NONBLOCK, (flags & O_NONBLOCK) ? 1 : 0) < 0) || >> + (fcntl_setfl(ret, O_SYNC, (flags & O_SYNC) ? 1 : 0) < 0)) { > > Yuck. That's a lot of syscalls (1 per fcntl_setfl() if they are already > set correctly, and 2 per fcntl_setfl() call if we are toggling each > one). It might be better to combine this into at most 2 fcntl() calls, > instead of a long sequence. > > I see your point. I'll call fcntl(F_GETFL) once to get the current flags, determine what needs to be set on/off, and then call fnctl(F_SETFL) once. In this case I won't be using fcntl_setfl() anymore. Do you want me to drop the fcntl_setfl() changes I made? Also, I noticed in the fnctl man page that F_SETFL: "On Linux this command can change only the O_APPEND, O_ASYNC, O_DIRECT, O_NOATIME, and O_NONBLOCK flags." So I'll only set/unset these flags. >> + /* Get the existing fd's flags */ >> + eflags = fcntl(fd, F_GETFL); >> + if (eflags == -1) { >> + return -1; >> + } >> + >> + if (((flags & O_RDWR) != (eflags & O_RDWR)) || >> + ((flags & O_RDONLY) != (eflags & O_RDONLY)) || >> + ((flags & O_WRONLY) != (eflags & O_WRONLY))) { > > Broken. O_RDWR, O_RDONLY, and O_WRONLY are NOT bitmasks, but are values > in the range of O_ACCMODE. In particular, O_RDONLY==0 on some platforms > (Linux), and ==1 on others (Hurd), and although POSIX recommends that > O_RDWR==(O_RDONLY|O_WRONLY) for any new systems, no one has really done > that except Hurd. > > A correct way to write this is: > > switch (flags & O_ACCMODE) { > case O_RDWR: > if ((eflags & O_ACCMODE) != O_RDWR) { > goto error; > break; > case O_RDONLY: > if ((eflags & O_ACCMODE) != O_RDONLY) { > goto error; > break; > case O_RDONLY: > if ((eflags & O_ACCMODE) != O_RDONLY) { > goto error; > break; > default: > goto error: > } > Thanks, I'll update this in v5. > [Technically, POSIX also requires O_ACCMODE to include O_SEARCH and > O_EXEC, although those two constants might be the same value; but right > now Linux has not yet implemented that bit; but unless qemu ever gains > the need to open executable binaries with O_EXEC or directories with > O_SEARCH, we probably don't have to worry about that aspect of O_ACCMODE > here.] > >> + errno = EACCES; >> + return -1; >> + } >> + >> + if (fcntl_setfl(fd, O_CLOEXEC, 1) < 0) { > > Again, broken. Besides, why are you attempting it both here and in > qemu_dup()? Shouldn't once be enough? > This is setting O_CLOEXEC for the inherited fd, and qemu_dup() is setting O_CLOEXEC for the dup'd fd, so I think this needs to remain (and use F_SETFD instead). >> + return -1; >> + } >> + >> + return qemu_dup(fd, flags); >
On 06/25/2012 08:24 AM, Corey Bryant wrote: >>> + if (fcntl_setfl(ret, O_CLOEXEC, (flags & O_CLOEXEC) ? 1 >>> : 0) < 0) { >> >> Broken. O_CLOEXEC _only_ affects open(); to change it on an existing >> fd, you have to use fcntl(F_GETFD/F_SETFD) (not F_GETFL/F_SETFL). >> >> > > I'll fix this in v5. Don't we already have qemu_set_cloexec() for this purpose? > > I see your point. I'll call fcntl(F_GETFL) once to get the current > flags, determine what needs to be set on/off, and then call > fnctl(F_SETFL) once. In this case I won't be using fcntl_setfl() > anymore. Do you want me to drop the fcntl_setfl() changes I made? Or maybe make fcntl_setfl() take a mask of bits to alter? You're right that it's not worth changing if you won't take advantage of the changes. > > Also, I noticed in the fnctl man page that F_SETFL: "On Linux this > command can change only the O_APPEND, O_ASYNC, O_DIRECT, O_NOATIME, and > O_NONBLOCK flags." So I'll only set/unset these flags. O_NDELAY is the obsolete spelling of O_NONBLOCK; which means the only other flags in your list not supported by Linux are O_LARGEFILE (which I said was pointless), O_NOCTTY (which only has an impact at open() and not later on, so it is not worth worrying about), and O_SYNC (so for that one, you should error out if not set correctly, as the difference between O_SYNC on vs. off is significant).
Am 25.06.2012 16:34, schrieb Eric Blake: >> Also, I noticed in the fnctl man page that F_SETFL: "On Linux this >> command can change only the O_APPEND, O_ASYNC, O_DIRECT, O_NOATIME, and >> O_NONBLOCK flags." So I'll only set/unset these flags. > > O_NDELAY is the obsolete spelling of O_NONBLOCK; which means the only > other flags in your list not supported by Linux are O_LARGEFILE (which I > said was pointless), O_NOCTTY (which only has an impact at open() and > not later on, so it is not worth worrying about), and O_SYNC (so for > that one, you should error out if not set correctly, as the difference > between O_SYNC on vs. off is significant). As far as I know, newer kernels can change O_SYNC with fcntl, so it's probably worth trying, but you'd have to check if it has really been set. Kevin
On 06/25/2012 10:34 AM, Eric Blake wrote: > On 06/25/2012 08:24 AM, Corey Bryant wrote: > >>>> + if (fcntl_setfl(ret, O_CLOEXEC, (flags & O_CLOEXEC) ? 1 >>>> : 0) < 0) { >>> >>> Broken. O_CLOEXEC _only_ affects open(); to change it on an existing >>> fd, you have to use fcntl(F_GETFD/F_SETFD) (not F_GETFL/F_SETFL). >>> >>> >> >> I'll fix this in v5. > > Don't we already have qemu_set_cloexec() for this purpose? > Yes, it looks that way. I'll use qemu_set_cloexec(). >> >> I see your point. I'll call fcntl(F_GETFL) once to get the current >> flags, determine what needs to be set on/off, and then call >> fnctl(F_SETFL) once. In this case I won't be using fcntl_setfl() >> anymore. Do you want me to drop the fcntl_setfl() changes I made? > > Or maybe make fcntl_setfl() take a mask of bits to alter? You're right > that it's not worth changing if you won't take advantage of the changes. > Good idea. I'll modify fcntl_setfl() to take a mask of bits to turn on or off. >> >> Also, I noticed in the fnctl man page that F_SETFL: "On Linux this >> command can change only the O_APPEND, O_ASYNC, O_DIRECT, O_NOATIME, and >> O_NONBLOCK flags." So I'll only set/unset these flags. > > O_NDELAY is the obsolete spelling of O_NONBLOCK; which means the only > other flags in your list not supported by Linux are O_LARGEFILE (which I > said was pointless), O_NOCTTY (which only has an impact at open() and > not later on, so it is not worth worrying about), and O_SYNC (so for > that one, you should error out if not set correctly, as the difference > between O_SYNC on vs. off is significant). > Ok I'll take this into account. Thanks.
On 06/25/2012 10:34 AM, Eric Blake wrote: > On 06/25/2012 08:24 AM, Corey Bryant wrote: > >>>> + if (fcntl_setfl(ret, O_CLOEXEC, (flags & O_CLOEXEC) ? 1 >>>> : 0) < 0) { >>> >>> Broken. O_CLOEXEC _only_ affects open(); to change it on an existing >>> fd, you have to use fcntl(F_GETFD/F_SETFD) (not F_GETFL/F_SETFL). >>> >>> >> >> I'll fix this in v5. > > Don't we already have qemu_set_cloexec() for this purpose? > Yes, it looks that way. I'll use qemu_set_cloexec(). >> >> I see your point. I'll call fcntl(F_GETFL) once to get the current >> flags, determine what needs to be set on/off, and then call >> fnctl(F_SETFL) once. In this case I won't be using fcntl_setfl() >> anymore. Do you want me to drop the fcntl_setfl() changes I made? > > Or maybe make fcntl_setfl() take a mask of bits to alter? You're right > that it's not worth changing if you won't take advantage of the changes. > Good idea. I'll modify fcntl_setfl() to take a mask of bits to turn on or off. >> >> Also, I noticed in the fnctl man page that F_SETFL: "On Linux this >> command can change only the O_APPEND, O_ASYNC, O_DIRECT, O_NOATIME, and >> O_NONBLOCK flags." So I'll only set/unset these flags. > > O_NDELAY is the obsolete spelling of O_NONBLOCK; which means the only > other flags in your list not supported by Linux are O_LARGEFILE (which I > said was pointless), O_NOCTTY (which only has an impact at open() and > not later on, so it is not worth worrying about), and O_SYNC (so for > that one, you should error out if not set correctly, as the difference > between O_SYNC on vs. off is significant). > Ok I'll take this into account. Thanks.
On 06/25/2012 11:26 AM, Kevin Wolf wrote: > Am 25.06.2012 16:34, schrieb Eric Blake: >>> Also, I noticed in the fnctl man page that F_SETFL: "On Linux this >>> command can change only the O_APPEND, O_ASYNC, O_DIRECT, O_NOATIME, and >>> O_NONBLOCK flags." So I'll only set/unset these flags. >> >> O_NDELAY is the obsolete spelling of O_NONBLOCK; which means the only >> other flags in your list not supported by Linux are O_LARGEFILE (which I >> said was pointless), O_NOCTTY (which only has an impact at open() and >> not later on, so it is not worth worrying about), and O_SYNC (so for >> that one, you should error out if not set correctly, as the difference >> between O_SYNC on vs. off is significant). > > As far as I know, newer kernels can change O_SYNC with fcntl, so it's > probably worth trying, but you'd have to check if it has really been set. > > Kevin > Ok I'll test it out and change O_SYNC if possible. Thanks.
diff --git a/cutils.c b/cutils.c index af308cd..f45d921 100644 --- a/cutils.c +++ b/cutils.c @@ -339,17 +339,33 @@ bool buffer_is_zero(const void *buf, size_t len) } #ifndef _WIN32 -/* Sets a specific flag */ -int fcntl_setfl(int fd, int flag) +/* Sets a specific flag on or off */ +int fcntl_setfl(int fd, int flag, int onoff) { int flags; + if (onoff != 0 && onoff != 1) { + return -EINVAL; + } + flags = fcntl(fd, F_GETFL); - if (flags == -1) + if (flags == -1) { return -errno; + } - if (fcntl(fd, F_SETFL, flags | flag) == -1) - return -errno; + if (onoff == 1) { + if ((flags & flag) != flag) { + if (fcntl(fd, F_SETFL, flags | flag) == -1) { + return -errno; + } + } + } else { + if ((flags & flag) == flag) { + if (fcntl(fd, F_SETFL, flags & ~flag) == -1) { + return -errno; + } + } + } return 0; } diff --git a/main-loop.c b/main-loop.c index eb3b6e6..644fcc3 100644 --- a/main-loop.c +++ b/main-loop.c @@ -75,11 +75,11 @@ static int qemu_event_init(void) if (err == -1) { return -errno; } - err = fcntl_setfl(fds[0], O_NONBLOCK); + err = fcntl_setfl(fds[0], O_NONBLOCK, 1); if (err < 0) { goto fail; } - err = fcntl_setfl(fds[1], O_NONBLOCK); + err = fcntl_setfl(fds[1], O_NONBLOCK, 1); if (err < 0) { goto fail; } @@ -154,7 +154,7 @@ static int qemu_signal_init(void) return -errno; } - fcntl_setfl(sigfd, O_NONBLOCK); + fcntl_setfl(sigfd, O_NONBLOCK, 1); qemu_set_fd_handler2(sigfd, NULL, sigfd_handler, NULL, (void *)(intptr_t)sigfd); diff --git a/osdep.c b/osdep.c index 3e6bada..a6fc758d 100644 --- a/osdep.c +++ b/osdep.c @@ -73,6 +73,63 @@ int qemu_madvise(void *addr, size_t len, int advice) #endif } +/* + * Dups an fd and sets the flags + */ +static int qemu_dup(int fd, int flags) +{ + int ret; + int serrno; + + if (flags & O_CLOEXEC) { + ret = fcntl(fd, F_DUPFD_CLOEXEC, 0); + if (ret == -1 && errno == EINVAL) { + ret = dup(fd); + if (ret == -1) { + goto fail; + } + if (fcntl_setfl(ret, O_CLOEXEC, (flags & O_CLOEXEC) ? 1 : 0) < 0) { + goto fail; + } + } + } else { + ret = dup(fd); + } + + if (ret == -1) { + goto fail; + } + + /* Truncate the file in the cases that open would truncate it */ + if (flags & O_TRUNC || + ((flags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))) { + if (ftruncate(ret, 0) == -1) { + goto fail; + } + } + + if ((fcntl_setfl(ret, O_APPEND, (flags & O_APPEND) ? 1 : 0) < 0) || + (fcntl_setfl(ret, O_ASYNC, (flags & O_ASYNC) ? 1 : 0) < 0) || + (fcntl_setfl(ret, O_DIRECT, (flags & O_DIRECT) ? 1 : 0) < 0) || + (fcntl_setfl(ret, O_LARGEFILE, (flags & O_LARGEFILE) ? 1 : 0) < 0) || + (fcntl_setfl(ret, O_NDELAY, (flags & O_NDELAY) ? 1 : 0) < 0) || + (fcntl_setfl(ret, O_NOATIME, (flags & O_NOATIME) ? 1 : 0) < 0) || + (fcntl_setfl(ret, O_NOCTTY, (flags & O_NOCTTY) ? 1 : 0) < 0) || + (fcntl_setfl(ret, O_NONBLOCK, (flags & O_NONBLOCK) ? 1 : 0) < 0) || + (fcntl_setfl(ret, O_SYNC, (flags & O_SYNC) ? 1 : 0) < 0)) { + goto fail; + } + + return ret; + +fail: + serrno = errno; + if (ret != -1) { + close(ret); + } + errno = serrno; + return -1; +} /* * Opens a file with FD_CLOEXEC set @@ -82,6 +139,40 @@ int qemu_open(const char *name, int flags, ...) int ret; int mode = 0; +#ifndef _WIN32 + const char *p; + + /* Attempt dup of fd for pre-opened file */ + if (strstart(name, "/dev/fd/", &p)) { + int fd; + int eflags; + + fd = qemu_parse_fd(p); + if (fd == -1) { + return -1; + } + + /* Get the existing fd's flags */ + eflags = fcntl(fd, F_GETFL); + if (eflags == -1) { + return -1; + } + + if (((flags & O_RDWR) != (eflags & O_RDWR)) || + ((flags & O_RDONLY) != (eflags & O_RDONLY)) || + ((flags & O_WRONLY) != (eflags & O_WRONLY))) { + errno = EACCES; + return -1; + } + + if (fcntl_setfl(fd, O_CLOEXEC, 1) < 0) { + return -1; + } + + return qemu_dup(fd, flags); + } +#endif + if (flags & O_CREAT) { va_list ap; diff --git a/qemu-common.h b/qemu-common.h index 91e0562..99cbbc5 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -144,7 +144,7 @@ int qemu_strnlen(const char *s, int max_len); time_t mktimegm(struct tm *tm); int qemu_fls(int i); int qemu_fdatasync(int fd); -int fcntl_setfl(int fd, int flag); +int fcntl_setfl(int fd, int flag, int onoff); int qemu_parse_fd(const char *param); /*
This patch adds support to qemu_open to dup(fd) a pre-opened file descriptor if the filename is of the format /dev/fd/X. This can be used when QEMU is restricted from opening files, and the management application opens files on QEMU's behalf. If the fd was passed to the monitor with the pass-fd command, it must be explicitly closed with the 'closefd' command when it is no longer required, in order to prevent fd leaks. Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- v2: -Get rid of file_open and move dup code to qemu_open (kwolf@redhat.com) -Use strtol wrapper instead of atoi (kwolf@redhat.com) v3: -Add note about fd leakage (eblake@redhat.com) v4 -Moved patch to be later in series (lcapitulino@redhat.com) -Update qemu_open to check access mode flags and set flags that can be set (eblake@redhat.com, kwolf@redhat.com) cutils.c | 26 +++++++++++++---- main-loop.c | 6 ++-- osdep.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qemu-common.h | 2 +- 4 files changed, 116 insertions(+), 9 deletions(-)