12:46 a.m.
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
12:46 a.m.
New subject: [libvirt] [PATCH] rpc: reformat the flow to make a bit more sense
Signed-off-by: Martin Kletzander <mkletzan(a)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
12:46 a.m.
New subject: [libvirt] [PATCH 2/5] remove redundant pidfile path constructions
Signed-off-by: Martin Kletzander <mkletzan(a)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
12:46 a.m.
New subject: [libvirt] [PATCH 3/5] util: fix potential leak in error codepath
Signed-off-by: Martin Kletzander <mkletzan(a)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
12:46 a.m.
New subject: [libvirt] [PATCH 4/5] util: get rid of unnecessary umask() call
Signed-off-by: Martin Kletzander <mkletzan(a)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
12:46 a.m.
New subject: [libvirt] [PATCH 5/5] rpc: make daemon spawning a bit more intelligent
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(a)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
6:27 p.m.
New subject: [libvirt] [PATCH 5/5] rpc: make daemon spawning a bit more intelligent
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(a)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;
4:12 a.m.
New subject: [libvirt] [PATCH 5/5] rpc: make daemon spawning a bit more intelligent
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(a)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
1:22 p.m.
New subject: [libvirt] [PATCH 5/5] rpc: make daemon spawning a bit more intelligent
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(a)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
8:33 a.m.
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
8:45 a.m.
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
8:55 a.m.
New subject: [libvirt] [PATCH 1/5] rpc: reformat the flow to make a bit more sense
Just remove useless "else". Best viewed with '-w'.
Signed-off-by: Martin Kletzander <mkletzan(a)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
6:23 a.m.
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(a)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
5:48 p.m.
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(a)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
2:53 a.m.
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(a)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
3720
days inactive
3728
days old
14 comments
4 participants
participants (4)
-
Cole Robinson -
John Ferlan -
Martin Kletzander -
Richard W.M. Jones