Series comparison

The qemu driver creates IPC sockets using absolute paths,
but under POSIX socket paths are constrained pretty tightly.
On systems with homedirs on an unusual mount point, like
network homedirs, or just particularly long usernames, this
could make starting VMs under qemu:///session impossible.

Resolves https://gitlab.com/libvirt/libvirt/-/issues/466

Signed-off-by: Nick Guenther <nick.guenther@polymtl.ca>
---
 src/qemu/qemu_command.c | 52 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index XXXXXXX..XXXXXXX 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -XXX,XX +XXX,XX @@
 #include "virmdev.h"
 #include "virutil.h"
 
+#include <libgen.h>
 #include <sys/stat.h>
 #include <fcntl.h>
 
@@ -XXX,XX +XXX,XX @@ qemuOpenChrChardevUNIXSocket(const virDomainChrSourceDef *dev)
     struct sockaddr_un addr;
     socklen_t addrlen = sizeof(addr);
     int fd;
+    char* wd = NULL;
+    char* socket_dir_c = NULL;
+    char* socket_name_c = NULL;
+    char* socket_dir = NULL;
+    char* socket_name = NULL;
+
+    /* The path length is limited to what fits in sockaddr_un.
+     * It's pretty short: 108 on Linux, and this is too easy to hit.
+     * Work around this limit by using a *relative path*.
+     *
+     * background: https://stackoverflow.com/questions/34829600/why-is-the-maximal-path-length-allowed-for-unix-sockets-on-linux-108
+     *
+     * docker added a different workaround: https://github.com/moby/moby/pull/13408
+     */
+    if ((wd = getcwd(NULL, 0)) == NULL) {
+        virReportSystemError(errno, "%s",
+                             _("Unable to get working directory"));
+	goto error;
+    }
+
+    socket_dir_c = strdup(dev->data.nix.path); // dirname edits the string given it, so it must be copied
+    socket_name_c = strdup(dev->data.nix.path);
+
+    socket_dir = dirname(socket_dir_c);
+    socket_name = basename(socket_name_c);
+
+    if (chdir(socket_dir) < 0) {
+        virReportSystemError(errno, "%s",
+                             _("Unable to get change to socket directory"));
+	goto error;
+    }
 
     if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) {
         virReportSystemError(errno, "%s",
@@ -XXX,XX +XXX,XX @@ qemuOpenChrChardevUNIXSocket(const virDomainChrSourceDef *dev)
 
     memset(&addr, 0, sizeof(addr));
     addr.sun_family = AF_UNIX;
-    if (virStrcpyStatic(addr.sun_path, dev->data.nix.path) < 0) {
+    if (virStrcpyStatic(addr.sun_path, socket_name) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("UNIX socket path '%1$s' too long"),
-                       dev->data.nix.path);
+                       _("UNIX socket name '%1$s' too long"),
+                       socket_name);
         goto error;
     }
 
@@ -XXX,XX +XXX,XX @@ qemuOpenChrChardevUNIXSocket(const virDomainChrSourceDef *dev)
     if (virFileUpdatePerm(dev->data.nix.path, 0002, 0664) < 0)
         goto error;
 
+    /* restore working directory */
+    if (chdir(wd) < 0) {
+        virReportSystemError(errno, "%s",
+                             _("Unable to restore working directory"));
+        goto error;
+    }
+
+    free(socket_name_c);
+    free(socket_dir_c);
+    free(wd);
+
     return fd;
 
  error:
+    if (socket_name_c != NULL) { free(socket_name_c); }
+    if (socket_dir_c != NULL) { free(socket_dir_c); }
+    if (wd != NULL) { free(wd); }
     VIR_FORCE_CLOSE(fd);
     return -1;
 }
-- 
2.34.1

v1: https://listman.redhat.com/archives/libvir-list/2023-April/239441.html

Address v1's cleanup bugs by using g_autofree and the concurrency bugs by
forking a subprocess before chdir(). It works!

I have been working on the other solution of putting paths in /var/run, from
https://listman.redhat.com/archives/libvir-list/2023-April/239536.html, but
so my attempts have ballooned out of control. I would like to offer this
improved version of my original patch in the meantime. Plus there's the
unclear impact of the other strategy on upgrades: https://listman.redhat.com/archives/libvir-list/2023-April/239470.html,
which means even once that's implemented it will take a long time to vet.
The chdir() approach cuts right to the point.

I'm hoping you'll accept both approaches as complementary and worth adopting.

Original issue: https://gitlab.com/libvirt/libvirt/-/issues/466

Cheers!

Nick Guenther (1):
  qemu: Allow sockets in long or deep paths.

src/qemu/qemu_command.c | 72 ++++++++++++++++++++++++++++++++---------
 1 file changed, 57 insertions(+), 15 deletions(-)

-- 
2.34.1

Resolves https://gitlab.com/libvirt/libvirt/-/issues/466

Signed-off-by: Nick Guenther <nick.guenther@polymtl.ca>
---
 src/qemu/qemu_command.c | 72 ++++++++++++++++++++++++++++++++---------
 1 file changed, 57 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index XXXXXXX..XXXXXXX 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -XXX,XX +XXX,XX @@ qemuBuildSCSIHostdevDevProps(const virDomainDef *def,
     return g_steal_pointer(&props);
 }
 
+struct qemuBindSocketData {
+    int fd;
+    char* path;
+};
+
+static int
+qemuBindSocket(pid_t ppid G_GNUC_UNUSED, void *opaque)
+{
+    /* The path length of a unix socket is limited to what fits in sockaddr_un.
+     * It's pretty short: 108 on Linux, and this is too easy to hit.
+     *
+     * Work around this limit by using a *relative path*, by chdir()ing first.
+     * But chdir() isn't thread-safe, so run it in a *subprocess* (this function)
+     * where the chdir() will be instantly forgotten once it has helped configure fd.
+     *
+     * background: https://stackoverflow.com/questions/34829600/why-is-the-maximal-path-length-allowed-for-unix-sockets-on-linux-108
+     */
+
+    g_autofree char *dir = NULL;
+    g_autofree char *name = NULL;
+
+    struct sockaddr_un addr;
+    struct qemuBindSocketData *data = opaque;
+
+    dir = g_path_get_dirname(data->path);
+    name = g_path_get_basename(data->path);
+
+    memset(&addr, 0, sizeof(addr));
+    addr.sun_family = AF_UNIX;
+    if (virStrcpyStatic(addr.sun_path, name) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("UNIX socket name '%1$s' too long"),
+                       name);
+        return -1;
+    }
+
+    if (chdir(dir) < 0) {
+        virReportSystemError(errno,
+                             _("Unable to chdir to containing directory '%1$s' while binding UNIX socket '%2$s'"),
+                             dir, name);
+        return -1;
+    }
+
+    if (bind(data->fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
+        virReportSystemError(errno,
+                             _("Unable to bind UNIX socket '%1$s/%2$s'"),
+                             dir, name);
+        return -1;
+    }
+
+    return 0;
+}
+
 int
 qemuOpenChrChardevUNIXSocket(const virDomainChrSourceDef *dev)
 {
-    struct sockaddr_un addr;
-    socklen_t addrlen = sizeof(addr);
     int fd;
+    struct qemuBindSocketData bindData;
 
     if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) {
         virReportSystemError(errno, "%s",
@@ -XXX,XX +XXX,XX @@ qemuOpenChrChardevUNIXSocket(const virDomainChrSourceDef *dev)
         goto error;
     }
 
-    memset(&addr, 0, sizeof(addr));
-    addr.sun_family = AF_UNIX;
-    if (virStrcpyStatic(addr.sun_path, dev->data.nix.path) < 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("UNIX socket path '%1$s' too long"),
-                       dev->data.nix.path);
-        goto error;
-    }
-
     if (unlink(dev->data.nix.path) < 0 && errno != ENOENT) {
         virReportSystemError(errno,
                              _("Unable to unlink %1$s"),
@@ -XXX,XX +XXX,XX @@ qemuOpenChrChardevUNIXSocket(const virDomainChrSourceDef *dev)
         goto error;
     }
 
-    if (bind(fd, (struct sockaddr *)&addr, addrlen) < 0) {
-        virReportSystemError(errno,
-                             _("Unable to bind to UNIX socket path '%1$s'"),
-                             dev->data.nix.path);
+    bindData.fd = fd;
+    bindData.path = dev->data.nix.path;
+    if (virProcessRunInFork(qemuBindSocket, &bindData) < 0) {
         goto error;
     }
 
-- 
2.34.1