On Wed, Jul 16, 2014 at 08:29:55PM +0200, Martin Kletzander wrote: [...] Note (not just) to self (but also reviewers): This function should probably set O_CLOEXEC flag on all the passed FDs, but I just realized that now and am not sure whether this was already done in virtlockd or not. Martin

Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/locking/lock_daemon.c | 2 +- src/rpc/virnetserverservice.c | 5 +++++ src/rpc/virnetserverservice.h | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index e9219d5..02d77e3 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -614,7 +614,7 @@ virLockDaemonSetupNetworkingSystemD(virNetServerPtr srv) #if WITH_GNUTLS NULL, #endif - false, 1))) + false, 0, 1))) return -1; if (virNetServerAddService(srv, svc, NULL) < 0) { diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index b50f57e..7892a95 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -131,6 +131,7 @@ virNetServerServiceNewFDOrUNIX(const char *path, tls, #endif readonly, + max_queued_clients, nrequests_client_max); } } @@ -263,6 +264,7 @@ virNetServerServicePtr virNetServerServiceNewFD(int fd, virNetTLSContextPtr tls, #endif bool readonly, + size_t max_queued_clients, size_t nrequests_client_max) { virNetServerServicePtr svc; @@ -290,6 +292,9 @@ virNetServerServicePtr virNetServerServiceNewFD(int fd, goto error; for (i = 0; i < svc->nsocks; i++) { + if (virNetSocketListen(svc->socks[i], max_queued_clients) < 0) + goto error; + /* IO callback is initially disabled, until we're ready * to deal with incoming clients */ if (virNetSocketAddIOCallback(svc->socks[i], diff --git a/src/rpc/virnetserverservice.h b/src/rpc/virnetserverservice.h index a1c8960..b1d6c2d 100644 --- a/src/rpc/virnetserverservice.h +++ b/src/rpc/virnetserverservice.h @@ -74,6 +74,7 @@ virNetServerServicePtr virNetServerServiceNewFD(int fd, virNetTLSContextPtr tls, # endif bool readonly, + size_t max_queued_clients, size_t nrequests_client_max); virNetServerServicePtr virNetServerServiceNewPostExecRestart(virJSONValuePtr object); -- 2.0.0

On Wed, Jul 16, 2014 at 08:29:58PM +0200, Martin Kletzander wrote: Shouldn't that be STDERR not STDIN since the passed FDs start at 3 not 1. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Even line like this: int asdf = i - somevar; was matched by the regex, so this patch adds '=' to the list of characters that break the regexp. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- cfg.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cfg.mk b/cfg.mk index baaab71..85a7060 100644 --- a/cfg.mk +++ b/cfg.mk @@ -568,7 +568,7 @@ sc_avoid_attribute_unused_in_header: $(_sc_search_regexp) sc_prohibit_int_ijk: - @prohibit='\<(int|unsigned) ([^(]* )*(i|j|k)\>(\s|,|;)' \ + @prohibit='\<(int|unsigned) ([^(=]* )*(i|j|k)\>(\s|,|;)' \ halt='use size_t, not int/unsigned int for loop vars i, j, k' \ $(_sc_search_regexp) -- 2.0.0

That sets a new flag, but that flag does mean the child will get LISTEN_FDS and LISTEN_PID environment variables properly set and passed FDs reordered so that it corresponds with LISTEN_FDS (they must start right after STDERR_FILENO). Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 99 ++++++++++++++++++++++++++++++++++++++++++++ src/util/vircommand.h | 4 +- tests/commanddata/test24.log | 7 ++++ tests/commandtest.c | 56 +++++++++++++++++++++++++ 5 files changed, 166 insertions(+), 1 deletion(-) create mode 100644 tests/commanddata/test24.log diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 138a9fa..3332952 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1165,6 +1165,7 @@ virCommandNewArgList; virCommandNewArgs; virCommandNonblockingFDs; virCommandPassFD; +virCommandPassListenFDs; virCommandRawStatus; virCommandRequireHandshake; virCommandRun; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index e775ba6..3b3e6f5 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -66,6 +66,7 @@ enum { VIR_EXEC_CLEAR_CAPS = (1 << 2), VIR_EXEC_RUN_SYNC = (1 << 3), VIR_EXEC_ASYNC_IO = (1 << 4), + VIR_EXEC_LISTEN_FDS = (1 << 5), }; typedef struct _virCommandFD virCommandFD; @@ -200,6 +201,78 @@ virCommandFDSet(virCommandPtr cmd, return 0; } +static void +virCommandReorderFDs(virCommandPtr cmd) +{ + int maxfd = 0; + int openmax = 0; + size_t i = 0; + + if (!cmd || cmd->has_error || !cmd->npassfd) + return; + + for (i = 0; i < cmd->npassfd; i++) + maxfd = MAX(cmd->passfd[i].fd, maxfd); + + openmax = sysconf(_SC_OPEN_MAX); + if (openmax < 0 || + maxfd + cmd->npassfd > openmax) + goto error; + + /* + * Simple two-pass sort, nothing fancy. This is not designed for + * anything else than passing around 2 FDs into the child. + * + * So first dup2() them somewhere else. + */ + for (i = 0; i < cmd->npassfd; i++) { + int newfd = maxfd + i + 1; + int oldfd = cmd->passfd[i].fd; + if (dup2(oldfd, newfd) != newfd) { + virReportSystemError(errno, + _("Cannot dup2() fd %d before " + "passing it to the child"), + oldfd); + goto error; + } + VIR_FORCE_CLOSE(cmd->passfd[i].fd); + } + + VIR_DEBUG("First reorder pass done"); + + /* + * And then dup2() them in orderly manner. + */ + for (i = 0; i < cmd->npassfd; i++) { + int newfd = STDERR_FILENO + i + 1; + int oldfd = maxfd + i + 1; + if (dup2(oldfd, newfd) != newfd) { + virReportSystemError(errno, + _("Cannot dup2() fd %d before " + "passing it to the child"), + oldfd); + goto error; + } + if (virSetInherit(newfd, true) < 0) { + virReportSystemError(errno, + _("Cannot set O_CLOEXEC on fd %d before " + "passing it to the child"), + newfd); + goto error; + } + VIR_FORCE_CLOSE(oldfd); + cmd->passfd[i].fd = newfd; + } + + VIR_DEBUG("Second reorder pass done"); + + return; + + error: + cmd->has_error = -1; + return; +} + #ifndef WIN32 /** @@ -678,6 +751,15 @@ virExec(virCommandPtr cmd) goto fork_error; } + if (cmd->flags & VIR_EXEC_LISTEN_FDS) { + virCommandReorderFDs(cmd); + virCommandAddEnvFormat(cmd, "LISTEN_PID=%u", getpid()); + virCommandAddEnvFormat(cmd, "LISTEN_FDS=%zu", cmd->npassfd); + + if (cmd->has_error) + goto fork_error; + } + /* Close logging again to ensure no FDs leak to child */ virLogReset(); @@ -919,6 +1001,23 @@ virCommandPassFD(virCommandPtr cmd, int fd, unsigned int flags) } /** + * virCommandPassListenFDs: + * @cmd: the command to modify + * + * Pass LISTEN_FDS and LISTEN_PID environment variables into the + * child. LISTEN_PID has the value of the child's PID and LISTEN_FDS + * is a number of passed file descriptors starting from 3. + */ +void +virCommandPassListenFDs(virCommandPtr cmd) +{ + if (!cmd || cmd->has_error) + return; + + cmd->flags |= VIR_EXEC_LISTEN_FDS; +} + +/** * virCommandSetPidFile: * @cmd: the command to modify * @pidfile: filename to use diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 8cdb31c..d3b286d 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -1,7 +1,7 @@ /* * vircommand.h: Child command execution * - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -60,6 +60,8 @@ void virCommandPassFD(virCommandPtr cmd, int fd, unsigned int flags); +void virCommandPassListenFDs(virCommandPtr cmd); + void virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) ATTRIBUTE_NONNULL(2); diff --git a/tests/commanddata/test24.log b/tests/commanddata/test24.log new file mode 100644 index 0000000..8670952 --- /dev/null +++ b/tests/commanddata/test24.log @@ -0,0 +1,7 @@ +FD:0 +FD:1 +FD:2 +FD:3 +FD:4 +DAEMON:yes +CWD:/ diff --git a/tests/commandtest.c b/tests/commandtest.c index ba823f7..b3287fa 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1032,6 +1032,61 @@ test23(const void *unused ATTRIBUTE_UNUSED) return ret; } +static int test24(const void *unused ATTRIBUTE_UNUSED) +{ + char *pidfile = virPidFileBuildPath(abs_builddir, "commandhelper"); + char *prefix = NULL; + int newfd1 = dup(STDERR_FILENO); + int newfd2 = dup(STDERR_FILENO); + int newfd3 = dup(STDERR_FILENO); + int ret = -1; + pid_t pid; + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + + if (!pidfile) + goto cleanup; + + if (VIR_CLOSE(newfd1) < 0) + printf("Cannot close fd %d\n", newfd1); + + virCommandSetPidFile(cmd, pidfile); + virCommandDaemonize(cmd); + virCommandPassFD(cmd, newfd2, VIR_COMMAND_PASS_FD_CLOSE_PARENT); + virCommandPassFD(cmd, newfd3, VIR_COMMAND_PASS_FD_CLOSE_PARENT); + virCommandPassListenFDs(cmd); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + goto cleanup; + } + + if (virPidFileRead(abs_builddir, "commandhelper", &pid) < 0) { + printf("cannot read pidfile\n"); + goto cleanup; + } + + if (virAsprintf(&prefix, + "ENV:LISTEN_FDS=2\nENV:LISTEN_PID=%u\n", + pid) < 0) + goto cleanup; + + while (kill(pid, 0) != -1) + usleep(100*1000); + + ret = checkoutput("test24", prefix); + + cleanup: + if (pidfile) + unlink(pidfile); + VIR_FREE(pidfile); + virCommandFree(cmd); + /* coverity[double_close] */ + VIR_FORCE_CLOSE(newfd2); + VIR_FORCE_CLOSE(newfd3); + return ret; +} + static void virCommandThreadWorker(void *opaque) { virCommandTestDataPtr test = opaque; @@ -1181,6 +1236,7 @@ mymain(void) DO_TEST(test21); DO_TEST(test22); DO_TEST(test23); + DO_TEST(test24); virMutexLock(&test->lock); if (test->running) { -- 2.0.0

On Wed, Jul 16, 2014 at 08:30:02PM +0200, Martin Kletzander wrote: [...] [...] I've finally found a way out of this. We can fork() and in the child do only umask() and bind(). It shouldn't be a problem that fstat() returns different mode for the socket than stat(), it should work everywhere and thanks to the fact that we do this pretty rarely and copy-on-write pages there shouldn't be significant impact. Is this acceptable? Martin