[libvirt] [PATCH 0/5] Fix problems caused by FD passing to session daemon
There were various problems introduced by the series on FD passing. The path for the socket was not created, the socket was not removed before binding it, etc. Let's see if it's any better this time. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604 Martin Kletzander (5): rpc: reformat the flow to make a bit more sense remove redundant pidfile path constructions util: fix potential leak in error codepath util: get rid of unnecessary umask() call rpc: make daemon spawning a bit more intelligent daemon/libvirtd.c | 41 ++------------ src/libvirt_private.syms | 1 + src/locking/lock_daemon.c | 42 ++------------- src/rpc/virnetsocket.c | 133 +++++++++++++++++++++++++++++++--------------- src/util/virpidfile.c | 48 ++++++++++++++++- src/util/virpidfile.h | 7 ++- 6 files changed, 151 insertions(+), 121 deletions(-) -- 2.1.0
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetsocket.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 586a0d7..42184bd 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -574,13 +574,14 @@ int virNetSocketNewConnectUNIX(const char *path, retry: if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + int status = 0; + pid_t pid = 0; + if (!spawnDaemon) { virReportSystemError(errno, _("Failed to connect socket to '%s'"), path); goto error; - } else { - int status = 0; - pid_t pid = 0; + } if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { virReportSystemError(errno, "%s", _("Failed to create socket")); @@ -634,7 +635,6 @@ int virNetSocketNewConnectUNIX(const char *path, if (virNetSocketForkDaemon(binary, passfd) < 0) goto error; } - } localAddr.len = sizeof(localAddr.data); if (getsockname(fd, &localAddr.data.sa, &localAddr.len) < 0) { -- 2.1.0
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- daemon/libvirtd.c | 41 ++++----------------------------------- src/libvirt_private.syms | 1 + src/locking/lock_daemon.c | 42 ++++------------------------------------ src/util/virpidfile.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++- src/util/virpidfile.h | 7 ++++++- 5 files changed, 63 insertions(+), 77 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 0503cd0..61f5486 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -251,41 +251,6 @@ static int daemonForkIntoBackground(const char *argv0) static int -daemonPidFilePath(bool privileged, - char **pidfile) -{ - if (privileged) { - if (VIR_STRDUP(*pidfile, LOCALSTATEDIR "/run/libvirtd.pid") < 0) - goto error; - } else { - char *rundir = NULL; - mode_t old_umask; - - if (!(rundir = virGetUserRuntimeDirectory())) - goto error; - - old_umask = umask(077); - if (virFileMakePath(rundir) < 0) { - umask(old_umask); - goto error; - } - umask(old_umask); - - if (virAsprintf(pidfile, "%s/libvirtd.pid", rundir) < 0) { - VIR_FREE(rundir); - goto error; - } - - VIR_FREE(rundir); - } - - return 0; - - error: - return -1; -} - -static int daemonUnixSocketPaths(struct daemonConfig *config, bool privileged, char **sockfile, @@ -1313,8 +1278,10 @@ int main(int argc, char **argv) { } if (!pid_file && - daemonPidFilePath(privileged, - &pid_file) < 0) { + virPidFileConstructPath(privileged, + LOCALSTATEDIR, + "libvirtd", + &pid_file) < 0) { VIR_ERROR(_("Can't determine pid file path.")); exit(EXIT_FAILURE); } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f136d24..9c6ab77 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1774,6 +1774,7 @@ virPCIIsVirtualFunction; virPidFileAcquire; virPidFileAcquirePath; virPidFileBuildPath; +virPidFileConstructPath; virPidFileDelete; virPidFileDeletePath; virPidFileRead; diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 02d77e3..fe7cfb8 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -366,42 +366,6 @@ virLockDaemonForkIntoBackground(const char *argv0) static int -virLockDaemonPidFilePath(bool privileged, - char **pidfile) -{ - if (privileged) { - if (VIR_STRDUP(*pidfile, LOCALSTATEDIR "/run/virtlockd.pid") < 0) - goto error; - } else { - char *rundir = NULL; - mode_t old_umask; - - if (!(rundir = virGetUserRuntimeDirectory())) - goto error; - - old_umask = umask(077); - if (virFileMakePath(rundir) < 0) { - umask(old_umask); - goto error; - } - umask(old_umask); - - if (virAsprintf(pidfile, "%s/virtlockd.pid", rundir) < 0) { - VIR_FREE(rundir); - goto error; - } - - VIR_FREE(rundir); - } - - return 0; - - error: - return -1; -} - - -static int virLockDaemonUnixSocketPaths(bool privileged, char **sockfile) { @@ -1283,8 +1247,10 @@ int main(int argc, char **argv) { } if (!pid_file && - virLockDaemonPidFilePath(privileged, - &pid_file) < 0) { + virPidFileConstructPath(privileged, + LOCALSTATEDIR, + "virtlockd", + &pid_file) < 0) { VIR_ERROR(_("Can't determine pid file path.")); exit(EXIT_FAILURE); } diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index 1d9a1c5..19ec103 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -1,7 +1,7 @@ /* * virpidfile.c: manipulation of pidfiles * - * Copyright (C) 2010-2012 Red Hat, Inc. + * Copyright (C) 2010-2012, 2014 Red Hat, Inc. * Copyright (C) 2006, 2007 Binary Karma * Copyright (C) 2006 Shuveb Hussain * @@ -521,3 +521,50 @@ int virPidFileRelease(const char *dir, VIR_FREE(pidfile); return rc; } + + +int +virPidFileConstructPath(bool privileged, + const char *statedir, + const char *progname, + char **pidfile) +{ + if (privileged) { + /* + * This is here just to allow calling this function with + * statedir == NULL; of course only when !privileged. + */ + if (!statedir) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("No statedir specified")); + goto cleanup; + } + if (virAsprintf(pidfile, "%s/run/%s.pid", statedir, progname) < 0) + goto cleanup; + } else { + char *rundir = NULL; + mode_t old_umask; + + if (!(rundir = virGetUserRuntimeDirectory())) + goto error; + + old_umask = umask(077); + if (virFileMakePath(rundir) < 0) { + umask(old_umask); + goto error; + } + umask(old_umask); + + if (virAsprintf(pidfile, "%s/%s.pid", rundir, progname) < 0) { + VIR_FREE(rundir); + goto error; + } + + VIR_FREE(rundir); + } + + return 0; + + error: + return -1; +} diff --git a/src/util/virpidfile.h b/src/util/virpidfile.h index 2720206..ca1dbff 100644 --- a/src/util/virpidfile.h +++ b/src/util/virpidfile.h @@ -1,7 +1,7 @@ /* * virpidfile.h: manipulation of pidfiles * - * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright (C) 2010-2011, 2014 Red Hat, Inc. * Copyright (C) 2006, 2007 Binary Karma * Copyright (C) 2006 Shuveb Hussain * @@ -69,4 +69,9 @@ int virPidFileRelease(const char *dir, const char *name, int fd); +int virPidFileConstructPath(bool privileged, + const char *statedir, + const char *progname, + char **pidfile); + #endif /* __VIR_PIDFILE_H__ */ -- 2.1.0
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virpidfile.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index 19ec103..dd29701 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -529,6 +529,9 @@ virPidFileConstructPath(bool privileged, const char *progname, char **pidfile) { + int ret = -1; + char *rundir = NULL; + if (privileged) { /* * This is here just to allow calling this function with @@ -542,29 +545,27 @@ virPidFileConstructPath(bool privileged, if (virAsprintf(pidfile, "%s/run/%s.pid", statedir, progname) < 0) goto cleanup; } else { - char *rundir = NULL; mode_t old_umask; if (!(rundir = virGetUserRuntimeDirectory())) - goto error; + goto cleanup; old_umask = umask(077); if (virFileMakePath(rundir) < 0) { umask(old_umask); - goto error; + goto cleanup; } umask(old_umask); if (virAsprintf(pidfile, "%s/%s.pid", rundir, progname) < 0) { VIR_FREE(rundir); - goto error; + goto cleanup; } - VIR_FREE(rundir); } - return 0; - - error: - return -1; + ret = 0; + cleanup: + VIR_FREE(rundir); + return ret; } -- 2.1.0
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virpidfile.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index dd29701..a3b8846 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -545,17 +545,15 @@ virPidFileConstructPath(bool privileged, if (virAsprintf(pidfile, "%s/run/%s.pid", statedir, progname) < 0) goto cleanup; } else { - mode_t old_umask; - if (!(rundir = virGetUserRuntimeDirectory())) goto cleanup; - old_umask = umask(077); - if (virFileMakePath(rundir) < 0) { - umask(old_umask); + if (virFileMakePathWithMode(rundir, 0700) < 0) { + virReportSystemError(errno, + _("Cannot create user runtime directory '%s'"), + rundir); goto cleanup; } - umask(old_umask); if (virAsprintf(pidfile, "%s/%s.pid", rundir, progname) < 0) { VIR_FREE(rundir); -- 2.1.0
This way it behaves more like the daemon itself does (acquiring a pidfile, deleting the socket before binding, etc.). Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetsocket.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 51 insertions(+), 6 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 42184bd..18a5a8f 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -51,9 +51,11 @@ #include "virlog.h" #include "virfile.h" #include "virthread.h" +#include "virpidfile.h" #include "virprobe.h" #include "virprocess.h" #include "virstring.h" +#include "dirname.h" #include "passfd.h" #if WITH_SSH2 @@ -544,7 +546,10 @@ int virNetSocketNewConnectUNIX(const char *path, const char *binary, virNetSocketPtr *retsock) { + char *binname = NULL; + char *pidpath = NULL; int fd, passfd = -1; + int pidfd = -1; virSocketAddr localAddr; virSocketAddr remoteAddr; @@ -583,16 +588,45 @@ int virNetSocketNewConnectUNIX(const char *path, goto error; } + if (!(binname = last_component(binary)) || binname[0] == '\0') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot determine basename for binary '%s'"), + binary); + goto error; + } + + if (virPidFileConstructPath(false, NULL, binname, &pidpath) < 0) + goto error; + + if ((pidfd = virPidFileAcquirePath(pidpath, false, getpid())) < 0) { + /* + * This can happen in a very rare case of two clients spawning two + * daemons at the same time, and the error in the logs that gets + * reset here can be a clue to some future debugging. + */ + virResetLastError(); + spawnDaemon = false; + goto retry; + } + if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { virReportSystemError(errno, "%s", _("Failed to create socket")); goto error; } /* - * We have to fork() here, because umask() is set - * per-process, chmod() is racy and fchmod() has undefined - * behaviour on sockets according to POSIX, so it doesn't - * work outside Linux. + * We already even acquired the pidfile, so noone else should be using + * @path right now. So we're OK to unlink it and paying attention to + * the return value makes no real sense here. Only if it's not an + * abstract socket, of course. + */ + if (path[0] != '@') + unlink(path); + + /* + * We have to fork() here, because umask() is set per-process, chmod() + * is racy and fchmod() has undefined behaviour on sockets according to + * POSIX, so it doesn't work outside Linux. */ if ((pid = virFork()) < 0) goto error; @@ -612,8 +646,9 @@ int virNetSocketNewConnectUNIX(const char *path, /* * OK, so the subprocces failed to bind() the socket. This may mean * that another daemon was starting at the same time and succeeded - * with its bind(). So we'll try connecting again, but this time - * without spawning the daemon. + * with its bind() (even though it should not happen because we + * using a pidfile for the race). So we'll try connecting again, + * but this time without spawning the daemon. */ spawnDaemon = false; goto retry; @@ -632,6 +667,12 @@ int virNetSocketNewConnectUNIX(const char *path, goto error; } + /* + * Do we need to eliminate the super-rare race here any more? It would + * need incorporating the following VIR_FORCE_CLOSE() into a + * virCommandHook inside a virNetSocketForkDaemon(). + */ + VIR_FORCE_CLOSE(pidfd); if (virNetSocketForkDaemon(binary, passfd) < 0) goto error; } @@ -648,8 +689,12 @@ int virNetSocketNewConnectUNIX(const char *path, return 0; error: + if (pidfd > 0) + virPidFileDeletePath(pidpath); + VIR_FREE(pidpath); VIR_FORCE_CLOSE(fd); VIR_FORCE_CLOSE(passfd); + VIR_FORCE_CLOSE(pidfd); if (spawnDaemon) unlink(path); return -1; -- 2.1.0
On 09/08/2014 01:46 AM, Martin Kletzander wrote:
This way it behaves more like the daemon itself does (acquiring a pidfile, deleting the socket before binding, etc.).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetsocket.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 51 insertions(+), 6 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 42184bd..18a5a8f 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -51,9 +51,11 @@ #include "virlog.h" #include "virfile.h" #include "virthread.h" +#include "virpidfile.h" #include "virprobe.h" #include "virprocess.h" #include "virstring.h" +#include "dirname.h" #include "passfd.h"
#if WITH_SSH2 @@ -544,7 +546,10 @@ int virNetSocketNewConnectUNIX(const char *path, const char *binary, virNetSocketPtr *retsock) { + char *binname = NULL; + char *pidpath = NULL; int fd, passfd = -1; + int pidfd = -1; virSocketAddr localAddr; virSocketAddr remoteAddr;
@@ -583,16 +588,45 @@ int virNetSocketNewConnectUNIX(const char *path, goto error; }
+ if (!(binname = last_component(binary)) || binname[0] == '\0') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot determine basename for binary '%s'"), + binary); + goto error; + } + + if (virPidFileConstructPath(false, NULL, binname, &pidpath) < 0) + goto error; + + if ((pidfd = virPidFileAcquirePath(pidpath, false, getpid())) < 0) { + /* + * This can happen in a very rare case of two clients spawning two + * daemons at the same time, and the error in the logs that gets + * reset here can be a clue to some future debugging. + */ + virResetLastError(); + spawnDaemon = false;
Since this is the failure path of connect(), the pidpath will be lost if the subsequent connect() succeeds.
+ goto retry; + } + if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { virReportSystemError(errno, "%s", _("Failed to create socket")); goto error; }
/* - * We have to fork() here, because umask() is set - * per-process, chmod() is racy and fchmod() has undefined - * behaviour on sockets according to POSIX, so it doesn't - * work outside Linux. + * We already even acquired the pidfile, so noone else should be using
s/noone/no one/
+ * @path right now. So we're OK to unlink it and paying attention to + * the return value makes no real sense here. Only if it's not an + * abstract socket, of course. + */ + if (path[0] != '@') + unlink(path); + + /* + * We have to fork() here, because umask() is set per-process, chmod() + * is racy and fchmod() has undefined behaviour on sockets according to + * POSIX, so it doesn't work outside Linux. */ if ((pid = virFork()) < 0) goto error; @@ -612,8 +646,9 @@ int virNetSocketNewConnectUNIX(const char *path, /* * OK, so the subprocces failed to bind() the socket. This may mean
Not related but, s/subprocces/subprocess [or I guess technically child]
* that another daemon was starting at the same time and succeeded - * with its bind(). So we'll try connecting again, but this time - * without spawning the daemon. + * with its bind() (even though it should not happen because we + * using a pidfile for the race). So we'll try connecting again, + * but this time without spawning the daemon. */ spawnDaemon = false; goto retry;
Like in the previous goto, since this is the failure path of a connect() and if the subsequent one succeeds, then 'pidfd', 'pidpath', 'passfd' are all leaked.
@@ -632,6 +667,12 @@ int virNetSocketNewConnectUNIX(const char *path, goto error; }
+ /* + * Do we need to eliminate the super-rare race here any more? It would + * need incorporating the following VIR_FORCE_CLOSE() into a + * virCommandHook inside a virNetSocketForkDaemon(). + */ + VIR_FORCE_CLOSE(pidfd);
Well - you caused me to look... it's interesting to note that "virNetSocketForkDaemon()" is bounded by "#ifndef WIN32", but it's a bit different here. Yes, I know it says *UNIX in the prototype, but yet virNetSocketNewConnectUNIX is bounded by "#ifdef HAVE_SYS_UN_H"... Of course any time there's a "timing window" available - something will eventually find it (whether or not there a full-moon, on the 13th of the month, when tides are extraordinarily high, or whatever other strange thing happens when software goes bad). Would another option to tighten the window be to pass the 'pidfd' (by reference to ensure a VIR_FREE(*pidfd); does what you expect) to the call and do the close "later" or closer to the condition causing the race? Optionally adjust the code to return 'cmd' and run from here assuming that's where the race is. Perhaps eblake will have an opinion and thoughts on the 'super-rare' condition...
if (virNetSocketForkDaemon(binary, passfd) < 0) goto error; } @@ -648,8 +689,12 @@ int virNetSocketNewConnectUNIX(const char *path, return 0;
error: + if (pidfd > 0) + virPidFileDeletePath(pidpath);
Is '0' a valid/possible 'pidfd'? Other places use pidfd != -1 and virPidFileReleasePath() which does the CLOSE as well and changes the order to avoid a different race. John
+ VIR_FREE(pidpath); VIR_FORCE_CLOSE(fd); VIR_FORCE_CLOSE(passfd); + VIR_FORCE_CLOSE(pidfd); if (spawnDaemon) unlink(path); return -1;
On Wed, Sep 10, 2014 at 07:27:40PM -0400, John Ferlan wrote:
On 09/08/2014 01:46 AM, Martin Kletzander wrote:
This way it behaves more like the daemon itself does (acquiring a pidfile, deleting the socket before binding, etc.).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetsocket.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 51 insertions(+), 6 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 42184bd..18a5a8f 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -51,9 +51,11 @@ #include "virlog.h" #include "virfile.h" #include "virthread.h" +#include "virpidfile.h" #include "virprobe.h" #include "virprocess.h" #include "virstring.h" +#include "dirname.h" #include "passfd.h"
#if WITH_SSH2 @@ -544,7 +546,10 @@ int virNetSocketNewConnectUNIX(const char *path, const char *binary, virNetSocketPtr *retsock) { + char *binname = NULL; + char *pidpath = NULL; int fd, passfd = -1; + int pidfd = -1; virSocketAddr localAddr; virSocketAddr remoteAddr;
@@ -583,16 +588,45 @@ int virNetSocketNewConnectUNIX(const char *path, goto error; }
+ if (!(binname = last_component(binary)) || binname[0] == '\0') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot determine basename for binary '%s'"), + binary); + goto error; + } + + if (virPidFileConstructPath(false, NULL, binname, &pidpath) < 0) + goto error; + + if ((pidfd = virPidFileAcquirePath(pidpath, false, getpid())) < 0) { + /* + * This can happen in a very rare case of two clients spawning two + * daemons at the same time, and the error in the logs that gets + * reset here can be a clue to some future debugging. + */ + virResetLastError(); + spawnDaemon = false;
Since this is the failure path of connect(), the pidpath will be lost if the subsequent connect() succeeds.
I added VIR_FREE(pidpath) between virPidFileAcquirePath() and the condition.
+ goto retry; + } + if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { virReportSystemError(errno, "%s", _("Failed to create socket")); goto error; }
/* - * We have to fork() here, because umask() is set - * per-process, chmod() is racy and fchmod() has undefined - * behaviour on sockets according to POSIX, so it doesn't - * work outside Linux. + * We already even acquired the pidfile, so noone else should be using
s/noone/no one/
OK;
+ * @path right now. So we're OK to unlink it and paying attention to + * the return value makes no real sense here. Only if it's not an + * abstract socket, of course. + */ + if (path[0] != '@') + unlink(path); + + /* + * We have to fork() here, because umask() is set per-process, chmod() + * is racy and fchmod() has undefined behaviour on sockets according to + * POSIX, so it doesn't work outside Linux. */ if ((pid = virFork()) < 0) goto error; @@ -612,8 +646,9 @@ int virNetSocketNewConnectUNIX(const char *path, /* * OK, so the subprocces failed to bind() the socket. This may mean
Not related but, s/subprocces/subprocess [or I guess technically child]
I fixed it to "child" when fixing the whole file, thanks for noticing that.
* that another daemon was starting at the same time and succeeded - * with its bind(). So we'll try connecting again, but this time - * without spawning the daemon. + * with its bind() (even though it should not happen because we + * using a pidfile for the race). So we'll try connecting again, + * but this time without spawning the daemon. */ spawnDaemon = false; goto retry;
Like in the previous goto, since this is the failure path of a connect() and if the subsequent one succeeds, then 'pidfd', 'pidpath', 'passfd' are all leaked.
Closing these in front of return 0; looked weird, so I closed them before the goto;
@@ -632,6 +667,12 @@ int virNetSocketNewConnectUNIX(const char *path, goto error; }
+ /* + * Do we need to eliminate the super-rare race here any more? It would + * need incorporating the following VIR_FORCE_CLOSE() into a + * virCommandHook inside a virNetSocketForkDaemon(). + */ + VIR_FORCE_CLOSE(pidfd);
Well - you caused me to look... it's interesting to note that "virNetSocketForkDaemon()" is bounded by "#ifndef WIN32", but it's a bit different here. Yes, I know it says *UNIX in the prototype, but yet virNetSocketNewConnectUNIX is bounded by "#ifdef HAVE_SYS_UN_H"...
I haven't heard from anyone having problem with this, even though it doesn't look very good, you're right. On the other hand what's the probability of someone out there compiling libvirt on non-win32 without UNIX sockets? :)
Of course any time there's a "timing window" available - something will eventually find it (whether or not there a full-moon, on the 13th of the month, when tides are extraordinarily high, or whatever other strange thing happens when software goes bad).
Yes, it will. Although there was a window between and this only affects starting virsh in session mode without any daemon, so it's not that serious, I guess.
Would another option to tighten the window be to pass the 'pidfd' (by reference to ensure a VIR_FREE(*pidfd); does what you expect) to the call and do the close "later" or closer to the condition causing the race? Optionally adjust the code to return 'cmd' and run from here assuming that's where the race is.
I could pass the fd into virNetSocketForkDaemon(), close it before starting daemon or just close it after fork (that would keep it opened in the process that's going to exec() libvirtd and thanks to O_CLOEXEC it would be closed after daemon is started. The window would still be there though, just few milliseconds smaller. Or we (and I *really* don't mean myself) could pass the fd to the pidfile into the daemon which would eliminate the problem completely.
Perhaps eblake will have an opinion and thoughts on the 'super-rare' condition...
if (virNetSocketForkDaemon(binary, passfd) < 0) goto error; } @@ -648,8 +689,12 @@ int virNetSocketNewConnectUNIX(const char *path, return 0;
error: + if (pidfd > 0) + virPidFileDeletePath(pidpath);
Is '0' a valid/possible 'pidfd'? Other places use pidfd != -1 and virPidFileReleasePath() which does the CLOSE as well and changes the order to avoid a different race.
Well, it's a valid FD, although it probably corresponds to console i/o or pty or what is it. It is, nevertheless, a valid file descriptor. That's one of the reasons I initialized it with -1; Sending a v2 with your comments addressed even though Cole ACK'd it (as it fixes the issue it shoul've fixed). Thanks for the review, Martin
On 09/08/2014 01:46 AM, Martin Kletzander wrote:
This way it behaves more like the daemon itself does (acquiring a pidfile, deleting the socket before binding, etc.).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetsocket.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 51 insertions(+), 6 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 42184bd..18a5a8f 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -51,9 +51,11 @@ #include "virlog.h" #include "virfile.h" #include "virthread.h" +#include "virpidfile.h" #include "virprobe.h" #include "virprocess.h" #include "virstring.h" +#include "dirname.h" #include "passfd.h"
#if WITH_SSH2 @@ -544,7 +546,10 @@ int virNetSocketNewConnectUNIX(const char *path, const char *binary, virNetSocketPtr *retsock) { + char *binname = NULL; + char *pidpath = NULL; int fd, passfd = -1; + int pidfd = -1; virSocketAddr localAddr; virSocketAddr remoteAddr;
@@ -583,16 +588,45 @@ int virNetSocketNewConnectUNIX(const char *path, goto error; }
+ if (!(binname = last_component(binary)) || binname[0] == '\0') { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot determine basename for binary '%s'"), + binary); + goto error; + } + + if (virPidFileConstructPath(false, NULL, binname, &pidpath) < 0) + goto error; + + if ((pidfd = virPidFileAcquirePath(pidpath, false, getpid())) < 0) { + /* + * This can happen in a very rare case of two clients spawning two + * daemons at the same time, and the error in the logs that gets + * reset here can be a clue to some future debugging. + */ + virResetLastError(); + spawnDaemon = false; + goto retry; + } + if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { virReportSystemError(errno, "%s", _("Failed to create socket")); goto error; }
/* - * We have to fork() here, because umask() is set - * per-process, chmod() is racy and fchmod() has undefined - * behaviour on sockets according to POSIX, so it doesn't - * work outside Linux. + * We already even acquired the pidfile, so noone else should be using + * @path right now. So we're OK to unlink it and paying attention to + * the return value makes no real sense here. Only if it's not an + * abstract socket, of course. + */ + if (path[0] != '@') + unlink(path); + + /* + * We have to fork() here, because umask() is set per-process, chmod() + * is racy and fchmod() has undefined behaviour on sockets according to + * POSIX, so it doesn't work outside Linux. */ if ((pid = virFork()) < 0) goto error; @@ -612,8 +646,9 @@ int virNetSocketNewConnectUNIX(const char *path, /* * OK, so the subprocces failed to bind() the socket. This may mean * that another daemon was starting at the same time and succeeded - * with its bind(). So we'll try connecting again, but this time - * without spawning the daemon. + * with its bind() (even though it should not happen because we + * using a pidfile for the race). So we'll try connecting again, + * but this time without spawning the daemon. */ spawnDaemon = false; goto retry; @@ -632,6 +667,12 @@ int virNetSocketNewConnectUNIX(const char *path, goto error; }
+ /* + * Do we need to eliminate the super-rare race here any more? It would + * need incorporating the following VIR_FORCE_CLOSE() into a + * virCommandHook inside a virNetSocketForkDaemon(). + */ + VIR_FORCE_CLOSE(pidfd); if (virNetSocketForkDaemon(binary, passfd) < 0) goto error; } @@ -648,8 +689,12 @@ int virNetSocketNewConnectUNIX(const char *path, return 0;
error: + if (pidfd > 0) + virPidFileDeletePath(pidpath); + VIR_FREE(pidpath); VIR_FORCE_CLOSE(fd); VIR_FORCE_CLOSE(passfd); + VIR_FORCE_CLOSE(pidfd); if (spawnDaemon) unlink(path); return -1;
Tested this, fixes the issue for me as well. Patch looks fine AFAICT, matches similar code stanzas in daemon/libvirtd.c. So ACK from me - Cole
On Mon, Sep 08, 2014 at 07:46:34AM +0200, Martin Kletzander wrote:
There were various problems introduced by the series on FD passing. The path for the socket was not created, the socket was not removed before binding it, etc. Let's see if it's any better this time.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604
Martin Kletzander (5): rpc: reformat the flow to make a bit more sense remove redundant pidfile path constructions util: fix potential leak in error codepath util: get rid of unnecessary umask() call rpc: make daemon spawning a bit more intelligent
daemon/libvirtd.c | 41 ++------------ src/libvirt_private.syms | 1 + src/locking/lock_daemon.c | 42 ++------------- src/rpc/virnetsocket.c | 133 +++++++++++++++++++++++++++++++--------------- src/util/virpidfile.c | 48 ++++++++++++++++- src/util/virpidfile.h | 7 ++- 6 files changed, 151 insertions(+), 121 deletions(-)
Patches 1 & 5 don't apply cleanly. Patch 5 is particularly quite different from current head. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On Mon, Sep 08, 2014 at 02:33:05PM +0100, Richard W.M. Jones wrote:
On Mon, Sep 08, 2014 at 07:46:34AM +0200, Martin Kletzander wrote:
There were various problems introduced by the series on FD passing. The path for the socket was not created, the socket was not removed before binding it, etc. Let's see if it's any better this time.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604
Martin Kletzander (5): rpc: reformat the flow to make a bit more sense remove redundant pidfile path constructions util: fix potential leak in error codepath util: get rid of unnecessary umask() call rpc: make daemon spawning a bit more intelligent
daemon/libvirtd.c | 41 ++------------ src/libvirt_private.syms | 1 + src/locking/lock_daemon.c | 42 ++------------- src/rpc/virnetsocket.c | 133 +++++++++++++++++++++++++++++++--------------- src/util/virpidfile.c | 48 ++++++++++++++++- src/util/virpidfile.h | 7 ++- 6 files changed, 151 insertions(+), 121 deletions(-)
Patches 1 & 5 don't apply cleanly. Patch 5 is particularly quite different from current head.
Patch 1 doesn't apply cleanly because I've sent *only* the version formatted with '-w' (and not the usual one) and patch 5 depends on that, so that's probably the reason for it. I'll send a normal 1/5 so it can be applied cleanly, thank you for noticing. Martin
Just remove useless "else". Best viewed with '-w'. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetsocket.c | 94 +++++++++++++++++++++++++------------------------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 586a0d7..42184bd 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -574,66 +574,66 @@ int virNetSocketNewConnectUNIX(const char *path, retry: if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + int status = 0; + pid_t pid = 0; + if (!spawnDaemon) { virReportSystemError(errno, _("Failed to connect socket to '%s'"), path); goto error; - } else { - int status = 0; - pid_t pid = 0; - - if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { - virReportSystemError(errno, "%s", _("Failed to create socket")); - goto error; - } + } - /* - * We have to fork() here, because umask() is set - * per-process, chmod() is racy and fchmod() has undefined - * behaviour on sockets according to POSIX, so it doesn't - * work outside Linux. - */ - if ((pid = virFork()) < 0) - goto error; + if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { + virReportSystemError(errno, "%s", _("Failed to create socket")); + goto error; + } - if (pid == 0) { - umask(0077); - if (bind(passfd, &remoteAddr.data.sa, remoteAddr.len) < 0) - _exit(EXIT_FAILURE); + /* + * We have to fork() here, because umask() is set + * per-process, chmod() is racy and fchmod() has undefined + * behaviour on sockets according to POSIX, so it doesn't + * work outside Linux. + */ + if ((pid = virFork()) < 0) + goto error; - _exit(EXIT_SUCCESS); - } + if (pid == 0) { + umask(0077); + if (bind(passfd, &remoteAddr.data.sa, remoteAddr.len) < 0) + _exit(EXIT_FAILURE); - if (virProcessWait(pid, &status, false) < 0) - goto error; + _exit(EXIT_SUCCESS); + } - if (status != EXIT_SUCCESS) { - /* - * OK, so the subprocces failed to bind() the socket. This may mean - * that another daemon was starting at the same time and succeeded - * with its bind(). So we'll try connecting again, but this time - * without spawning the daemon. - */ - spawnDaemon = false; - goto retry; - } + if (virProcessWait(pid, &status, false) < 0) + goto error; - if (listen(passfd, 0) < 0) { - virReportSystemError(errno, "%s", - _("Failed to listen on socket that's about " - "to be passed to the daemon")); - goto error; - } + if (status != EXIT_SUCCESS) { + /* + * OK, so the subprocces failed to bind() the socket. This may mean + * that another daemon was starting at the same time and succeeded + * with its bind(). So we'll try connecting again, but this time + * without spawning the daemon. + */ + spawnDaemon = false; + goto retry; + } - if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { - virReportSystemError(errno, _("Failed to connect socket to '%s'"), - path); - goto error; - } + if (listen(passfd, 0) < 0) { + virReportSystemError(errno, "%s", + _("Failed to listen on socket that's about " + "to be passed to the daemon")); + goto error; + } - if (virNetSocketForkDaemon(binary, passfd) < 0) - goto error; + if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + virReportSystemError(errno, _("Failed to connect socket to '%s'"), + path); + goto error; } + + if (virNetSocketForkDaemon(binary, passfd) < 0) + goto error; } localAddr.len = sizeof(localAddr.data); -- 2.1.0
On Mon, Sep 08, 2014 at 07:46:34AM +0200, Martin Kletzander wrote:
There were various problems introduced by the series on FD passing. The path for the socket was not created, the socket was not removed before binding it, etc. Let's see if it's any better this time.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604
Martin Kletzander (5): rpc: reformat the flow to make a bit more sense remove redundant pidfile path constructions util: fix potential leak in error codepath util: get rid of unnecessary umask() call rpc: make daemon spawning a bit more intelligent
daemon/libvirtd.c | 41 ++------------ src/libvirt_private.syms | 1 + src/locking/lock_daemon.c | 42 ++------------- src/rpc/virnetsocket.c | 133 +++++++++++++++++++++++++++++++--------------- src/util/virpidfile.c | 48 ++++++++++++++++- src/util/virpidfile.h | 7 ++- 6 files changed, 151 insertions(+), 121 deletions(-)
I tested the patch series and it appears to fix the problem in bug 1138604. I used the test procedure described here: https://bugzilla.redhat.com/show_bug.cgi?id=1138604#c2 Tested-by: Richard W.M. Jones <rjones@redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On 09/08/2014 01:46 AM, Martin Kletzander wrote:
There were various problems introduced by the series on FD passing. The path for the socket was not created, the socket was not removed before binding it, etc. Let's see if it's any better this time.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604
Martin Kletzander (5): rpc: reformat the flow to make a bit more sense remove redundant pidfile path constructions util: fix potential leak in error codepath util: get rid of unnecessary umask() call rpc: make daemon spawning a bit more intelligent
daemon/libvirtd.c | 41 ++------------ src/libvirt_private.syms | 1 + src/locking/lock_daemon.c | 42 ++------------- src/rpc/virnetsocket.c | 133 +++++++++++++++++++++++++++++++--------------- src/util/virpidfile.c | 48 ++++++++++++++++- src/util/virpidfile.h | 7 ++- 6 files changed, 151 insertions(+), 121 deletions(-)
-- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
ACK patches 1-4, although they are a little light on patch descriptions beyond the commit message. John
On Wed, Sep 10, 2014 at 06:48:48PM -0400, John Ferlan wrote:
On 09/08/2014 01:46 AM, Martin Kletzander wrote:
There were various problems introduced by the series on FD passing. The path for the socket was not created, the socket was not removed before binding it, etc. Let's see if it's any better this time.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604
Martin Kletzander (5): rpc: reformat the flow to make a bit more sense remove redundant pidfile path constructions util: fix potential leak in error codepath util: get rid of unnecessary umask() call rpc: make daemon spawning a bit more intelligent
daemon/libvirtd.c | 41 ++------------ src/libvirt_private.syms | 1 + src/locking/lock_daemon.c | 42 ++------------- src/rpc/virnetsocket.c | 133 +++++++++++++++++++++++++++++++--------------- src/util/virpidfile.c | 48 ++++++++++++++++- src/util/virpidfile.h | 7 ++- 6 files changed, 151 insertions(+), 121 deletions(-)
-- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
ACK patches 1-4, although they are a little light on patch descriptions beyond the commit message.
I thought they are pretty self-explanatory, I just wanted to split them to ease the review and they ended up being pretty small. I'll try to make the commit message more explanatory next time. Anyway, thanks for the review, 1-4 are pushed now. Martin
participants (4)
-
Cole Robinson -
John Ferlan -
Martin Kletzander -
Richard W.M. Jones