[PATCH v2 1/2] launch: add internal helper for socket paths creation
Introduce an internal helper to create paths for sockets -- will be useful for changing later the logic for placing sockets. Futhermore, check that the length of sockets won't overflow the buffer for their filenames. --- src/guestfs-internal.h | 1 + src/launch-direct.c | 4 +++- src/launch-libvirt.c | 10 ++++++---- src/launch.c | 17 +++++++++++++++++ 4 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index 5ecd322..bff9f64 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -782,6 +782,7 @@ extern void guestfs_int_launch_send_progress (guestfs_h *g, int perdozen); extern char *guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_dev, int flags); #define APPLIANCE_COMMAND_LINE_IS_TCG 1 const char *guestfs_int_get_cpu_model (int kvm); +int guestfs_int_create_socketname (guestfs_h *g, const char *filename, char (*sockname)[UNIX_PATH_MAX]); extern void guestfs_int_register_backend (const char *name, const struct backend_ops *); extern int guestfs_int_set_backend (guestfs_h *g, const char *method); diff --git a/src/launch-direct.c b/src/launch-direct.c index b8e453d..a81d4b3 100644 --- a/src/launch-direct.c +++ b/src/launch-direct.c @@ -295,7 +295,9 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) /* Using virtio-serial, we need to create a local Unix domain socket * for qemu to connect to. */ - snprintf (data->guestfsd_sock, sizeof data->guestfsd_sock, "%s/guestfsd.sock", g->tmpdir); + if (guestfs_int_create_socketname (g, "guestfsd.sock", + &data->guestfsd_sock) == -1) + goto cleanup0; daemon_accept_sock = socket (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0); if (daemon_accept_sock == -1) { diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c index 8a5d93e..376bd80 100644 --- a/src/launch-libvirt.c +++ b/src/launch-libvirt.c @@ -395,8 +395,9 @@ launch_libvirt (guestfs_h *g, void *datav, const char *libvirt_uri) /* Using virtio-serial, we need to create a local Unix domain socket * for qemu to connect to. */ - snprintf (data->guestfsd_path, sizeof data->guestfsd_path, - "%s/guestfsd.sock", g->tmpdir); + if (guestfs_int_create_socketname (g, "guestfsd.sock", + &data->guestfsd_path) == -1) + goto cleanup; set_socket_create_context (g); @@ -421,8 +422,9 @@ launch_libvirt (guestfs_h *g, void *datav, const char *libvirt_uri) } /* For the serial console. */ - snprintf (data->console_path, sizeof data->console_path, - "%s/console.sock", g->tmpdir); + if (guestfs_int_create_socketname (g, "console.sock", + &data->console_path) == -1) + goto cleanup; console_sock = socket (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0); if (console_sock == -1) { diff --git a/src/launch.c b/src/launch.c index f59818f..60f02a7 100644 --- a/src/launch.c +++ b/src/launch.c @@ -418,6 +418,23 @@ guestfs_int_get_cpu_model (int kvm) #endif } +/* Create the path for a socket with the selected filename in the + * tmpdir. + */ +int +guestfs_int_create_socketname (guestfs_h *g, const char *filename, + char (*sockpath)[UNIX_PATH_MAX]) +{ + if (strlen (g->tmpdir) + 1 + strlen (filename) > UNIX_PATH_MAX-1) { + error (g, _("socket path too long: %s/%s"), g->tmpdir, filename); + return -1; + } + + snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", g->tmpdir, filename); + + return 0; +} + /* glibc documents, but does not actually implement, a 'getumask(3)' * call. This implements a thread-safe way to get the umask. Note * this is only called when g->verbose is true and after g->tmpdir -- 2.5.0
Introduce a new read-only API to get a path where to store temporary sockets: this is different from tmpdir, as we need short paths for sockets (due to sockaddr_un::sun_path), and it is either XDG_RUNTIME_DIR if set, or /tmp; adapt guestfs_int_create_socketname to create sockets in that location. Furthermore, print sockdir and XDG_RUNTIME_DIR in test-tool for debugging. --- fish/guestfish.pod | 11 +++++++++++ generator/actions.ml | 16 ++++++++++++++++ src/guestfs-internal.h | 7 ++++++- src/guestfs.pod | 11 +++++++++++ src/handle.c | 9 ++++++++- src/launch.c | 9 ++++++--- src/tmpdirs.c | 33 +++++++++++++++++++++++++++++++++ test-tool/test-tool.c | 6 ++++++ 8 files changed, 97 insertions(+), 5 deletions(-) diff --git a/fish/guestfish.pod b/fish/guestfish.pod index c6f5663..bbeea82 100644 --- a/fish/guestfish.pod +++ b/fish/guestfish.pod @@ -1519,6 +1519,17 @@ about kernel selection, see L<supermin(1)>. See L</LIBGUESTFS_CACHEDIR>, L</LIBGUESTFS_TMPDIR>. +=item XDG_RUNTIME_DIR + +This directory represents a user-specific directory for storing +non-essential runtime files. + +If it is set, then is used to store temporary sockets. Otherwise, +F</tmp> is used. + +See also L</get-sockdir>, +L<http://www.freedesktop.org/wiki/Specifications/basedir-spec/>. + =back =head1 FILES diff --git a/generator/actions.ml b/generator/actions.ml index 24c6eb7..4078082 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -3504,6 +3504,22 @@ call it returns a simple true/false boolean result, instead of throwing an exception if a feature is not found. For other documentation see C<guestfs_available>." }; + { defaults with + name = "get_sockdir"; added = (1, 33, 8); + style = RString "sockdir", [], []; + blocking = false; + shortdesc = "get the temporary directory for sockets"; + longdesc = "\ +Get the directory used by the handle to store temporary socket files. + +This is different from C<guestfs_tmpdir>, as we need shorter paths for +sockets (due to the limited buffers of filenames for UNIX sockets), +and C<guestfs_tmpdir> may be too long for them. + +The environment variable C<XDG_RUNTIME_DIR> controls the default +value: If C<XDG_RUNTIME_DIR> is set, then that is the default. +Else F</tmp> is the default." }; + ] (* daemon_functions are any functions which cause some action diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index bff9f64..6e441e4 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -434,8 +434,10 @@ struct guestfs_h * handle, you have to call guestfs_int_lazy_make_tmpdir. */ char *tmpdir; - /* Environment variables that affect tmpdir/cachedir locations. */ + char *sockdir; + /* Environment variables that affect tmpdir/cachedir/sockdir locations. */ char *env_tmpdir; /* $TMPDIR (NULL if not set) */ + char *env_runtimedir; /* $XDG_RUNTIME_DIR (NULL if not set)*/ char *int_tmpdir; /* $LIBGUESTFS_TMPDIR or guestfs_set_tmpdir or NULL */ char *int_cachedir; /* $LIBGUESTFS_CACHEDIR or guestfs_set_cachedir or NULL */ @@ -757,8 +759,11 @@ extern void guestfs_int_call_callbacks_array (guestfs_h *g, uint64_t event, cons /* tmpdirs.c */ extern int guestfs_int_set_env_tmpdir (guestfs_h *g, const char *tmpdir); +extern int guestfs_int_set_env_runtimedir (guestfs_h *g, const char *runtimedir); extern int guestfs_int_lazy_make_tmpdir (guestfs_h *g); +extern int guestfs_int_lazy_make_sockdir (guestfs_h *g); extern void guestfs_int_remove_tmpdir (guestfs_h *g); +extern void guestfs_int_remove_sockdir (guestfs_h *g); extern void guestfs_int_recursive_remove_dir (guestfs_h *g, const char *dir); /* whole-file.c */ diff --git a/src/guestfs.pod b/src/guestfs.pod index a82d060..2a199c0 100644 --- a/src/guestfs.pod +++ b/src/guestfs.pod @@ -3482,6 +3482,17 @@ about kernel selection, see L<supermin(1)>. See L</LIBGUESTFS_CACHEDIR>, L</LIBGUESTFS_TMPDIR>. +=item XDG_RUNTIME_DIR + +This directory represents a user-specific directory for storing +non-essential runtime files. + +If it is set, then is used to store temporary sockets. Otherwise, +F</tmp> is used. + +See also L</get-sockdir>, +L<http://www.freedesktop.org/wiki/Specifications/basedir-spec/>. + =back =head1 SEE ALSO diff --git a/src/handle.c b/src/handle.c index 947818c..25d3c99 100644 --- a/src/handle.c +++ b/src/handle.c @@ -273,6 +273,10 @@ parse_environment (guestfs_h *g, return -1; } + str = do_getenv (data, "XDG_RUNTIME_DIR"); + if (guestfs_int_set_env_runtimedir (g, str) == -1) + return -1; + return 0; } @@ -347,8 +351,9 @@ guestfs_close (guestfs_h *g) if (g->test_fp != NULL) fclose (g->test_fp); - /* Remove temporary directory. */ + /* Remove temporary directories. */ guestfs_int_remove_tmpdir (g); + guestfs_int_remove_sockdir (g); /* Mark the handle as dead and then free up all memory. */ g->state = NO_HANDLE; @@ -377,7 +382,9 @@ guestfs_close (guestfs_h *g) if (g->pda) hash_free (g->pda); free (g->tmpdir); + free (g->sockdir); free (g->env_tmpdir); + free (g->env_runtimedir); free (g->int_tmpdir); free (g->int_cachedir); free (g->last_error); diff --git a/src/launch.c b/src/launch.c index 60f02a7..9273c58 100644 --- a/src/launch.c +++ b/src/launch.c @@ -425,12 +425,15 @@ int guestfs_int_create_socketname (guestfs_h *g, const char *filename, char (*sockpath)[UNIX_PATH_MAX]) { - if (strlen (g->tmpdir) + 1 + strlen (filename) > UNIX_PATH_MAX-1) { - error (g, _("socket path too long: %s/%s"), g->tmpdir, filename); + if (guestfs_int_lazy_make_sockdir (g) == -1) + return -1; + + if (strlen (g->sockdir) + 1 + strlen (filename) > UNIX_PATH_MAX-1) { + error (g, _("socket path too long: %s/%s"), g->sockdir, filename); return -1; } - snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", g->tmpdir, filename); + snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", g->sockdir, filename); return 0; } diff --git a/src/tmpdirs.c b/src/tmpdirs.c index 2ae9c74..e66ab9c 100644 --- a/src/tmpdirs.c +++ b/src/tmpdirs.c @@ -76,6 +76,12 @@ guestfs_int_set_env_tmpdir (guestfs_h *g, const char *tmpdir) } int +guestfs_int_set_env_runtimedir (guestfs_h *g, const char *runtimedir) +{ + return set_abs_path (g, runtimedir, &g->env_runtimedir); +} + +int guestfs_impl_set_tmpdir (guestfs_h *g, const char *tmpdir) { return set_abs_path (g, tmpdir, &g->int_tmpdir); @@ -119,6 +125,20 @@ guestfs_impl_get_cachedir (guestfs_h *g) return safe_strdup (g, str); } +/* Note this actually calculates the sockdir, so it never returns NULL. */ +char * +guestfs_impl_get_sockdir (guestfs_h *g) +{ + const char *str; + + if (g->env_runtimedir) + str = g->env_runtimedir; + else + str = "/tmp"; + + return safe_strdup (g, str); +} + static int lazy_make_tmpdir (guestfs_h *g, char *(*getdir) (guestfs_h *g), char **dest) { @@ -145,6 +165,12 @@ guestfs_int_lazy_make_tmpdir (guestfs_h *g) return lazy_make_tmpdir (g, guestfs_get_tmpdir, &g->tmpdir); } +int +guestfs_int_lazy_make_sockdir (guestfs_h *g) +{ + return lazy_make_tmpdir (g, guestfs_get_sockdir, &g->sockdir); +} + /* Recursively remove a temporary directory. If removal fails, just * return (it's a temporary directory so it'll eventually be cleaned * up by a temp cleaner). This is done using "rm -rf" because that's @@ -167,3 +193,10 @@ guestfs_int_remove_tmpdir (guestfs_h *g) if (g->tmpdir) guestfs_int_recursive_remove_dir (g, g->tmpdir); } + +void +guestfs_int_remove_sockdir (guestfs_h *g) +{ + if (g->sockdir) + guestfs_int_recursive_remove_dir (g, g->sockdir); +} diff --git a/test-tool/test-tool.c b/test-tool/test-tool.c index 9d23d6d..495316b 100644 --- a/test-tool/test-tool.c +++ b/test-tool/test-tool.c @@ -210,6 +210,9 @@ main (int argc, char *argv[]) p = getenv ("PATH"); if (p) printf ("PATH=%s\n", p); + p = getenv ("XDG_RUNTIME_DIR"); + if (p) + printf ("XDG_RUNTIME_DIR=%s\n", p); /* Print SELinux mode (don't worry if this fails, or if the command * doesn't even exist). @@ -255,6 +258,9 @@ main (int argc, char *argv[]) printf ("guestfs_get_recovery_proc: %d\n", guestfs_get_recovery_proc (g)); printf ("guestfs_get_selinux: %d\n", guestfs_get_selinux (g)); printf ("guestfs_get_smp: %d\n", guestfs_get_smp (g)); + p = guestfs_get_sockdir (g); + printf ("guestfs_get_sockdir: %s\n", p ? : "(null)"); + free (p); p = guestfs_get_tmpdir (g); printf ("guestfs_get_tmpdir: %s\n", p ? : "(null)"); free (p); -- 2.5.0
On Wed, Feb 03, 2016 at 01:17:42PM +0100, Pino Toscano wrote:
Introduce a new read-only API to get a path where to store temporary sockets: this is different from tmpdir, as we need short paths for sockets (due to sockaddr_un::sun_path), and it is either XDG_RUNTIME_DIR if set, or /tmp; adapt guestfs_int_create_socketname to create sockets in that location.
Furthermore, print sockdir and XDG_RUNTIME_DIR in test-tool for debugging. --- fish/guestfish.pod | 11 +++++++++++ generator/actions.ml | 16 ++++++++++++++++ src/guestfs-internal.h | 7 ++++++- src/guestfs.pod | 11 +++++++++++ src/handle.c | 9 ++++++++- src/launch.c | 9 ++++++--- src/tmpdirs.c | 33 +++++++++++++++++++++++++++++++++ test-tool/test-tool.c | 6 ++++++ 8 files changed, 97 insertions(+), 5 deletions(-)
diff --git a/fish/guestfish.pod b/fish/guestfish.pod index c6f5663..bbeea82 100644 --- a/fish/guestfish.pod +++ b/fish/guestfish.pod @@ -1519,6 +1519,17 @@ about kernel selection, see L<supermin(1)>.
See L</LIBGUESTFS_CACHEDIR>, L</LIBGUESTFS_TMPDIR>.
+=item XDG_RUNTIME_DIR + +This directory represents a user-specific directory for storing +non-essential runtime files. + +If it is set, then is used to store temporary sockets. Otherwise, +F</tmp> is used. + +See also L</get-sockdir>, +L<http://www.freedesktop.org/wiki/Specifications/basedir-spec/>. + =back
=head1 FILES diff --git a/generator/actions.ml b/generator/actions.ml index 24c6eb7..4078082 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -3504,6 +3504,22 @@ call it returns a simple true/false boolean result, instead of throwing an exception if a feature is not found. For other documentation see C<guestfs_available>." };
+ { defaults with + name = "get_sockdir"; added = (1, 33, 8); + style = RString "sockdir", [], []; + blocking = false; + shortdesc = "get the temporary directory for sockets"; + longdesc = "\ +Get the directory used by the handle to store temporary socket files. + +This is different from C<guestfs_tmpdir>, as we need shorter paths for +sockets (due to the limited buffers of filenames for UNIX sockets), +and C<guestfs_tmpdir> may be too long for them. + +The environment variable C<XDG_RUNTIME_DIR> controls the default +value: If C<XDG_RUNTIME_DIR> is set, then that is the default. +Else F</tmp> is the default." }; + ]
(* daemon_functions are any functions which cause some action diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index bff9f64..6e441e4 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -434,8 +434,10 @@ struct guestfs_h * handle, you have to call guestfs_int_lazy_make_tmpdir. */ char *tmpdir; - /* Environment variables that affect tmpdir/cachedir locations. */ + char *sockdir; + /* Environment variables that affect tmpdir/cachedir/sockdir locations. */ char *env_tmpdir; /* $TMPDIR (NULL if not set) */ + char *env_runtimedir; /* $XDG_RUNTIME_DIR (NULL if not set)*/ char *int_tmpdir; /* $LIBGUESTFS_TMPDIR or guestfs_set_tmpdir or NULL */ char *int_cachedir; /* $LIBGUESTFS_CACHEDIR or guestfs_set_cachedir or NULL */
@@ -757,8 +759,11 @@ extern void guestfs_int_call_callbacks_array (guestfs_h *g, uint64_t event, cons
/* tmpdirs.c */ extern int guestfs_int_set_env_tmpdir (guestfs_h *g, const char *tmpdir); +extern int guestfs_int_set_env_runtimedir (guestfs_h *g, const char *runtimedir); extern int guestfs_int_lazy_make_tmpdir (guestfs_h *g); +extern int guestfs_int_lazy_make_sockdir (guestfs_h *g); extern void guestfs_int_remove_tmpdir (guestfs_h *g); +extern void guestfs_int_remove_sockdir (guestfs_h *g); extern void guestfs_int_recursive_remove_dir (guestfs_h *g, const char *dir);
/* whole-file.c */ diff --git a/src/guestfs.pod b/src/guestfs.pod index a82d060..2a199c0 100644 --- a/src/guestfs.pod +++ b/src/guestfs.pod @@ -3482,6 +3482,17 @@ about kernel selection, see L<supermin(1)>.
See L</LIBGUESTFS_CACHEDIR>, L</LIBGUESTFS_TMPDIR>.
+=item XDG_RUNTIME_DIR + +This directory represents a user-specific directory for storing +non-essential runtime files. + +If it is set, then is used to store temporary sockets. Otherwise, +F</tmp> is used. + +See also L</get-sockdir>, +L<http://www.freedesktop.org/wiki/Specifications/basedir-spec/>. + =back
=head1 SEE ALSO diff --git a/src/handle.c b/src/handle.c index 947818c..25d3c99 100644 --- a/src/handle.c +++ b/src/handle.c @@ -273,6 +273,10 @@ parse_environment (guestfs_h *g, return -1; }
+ str = do_getenv (data, "XDG_RUNTIME_DIR"); + if (guestfs_int_set_env_runtimedir (g, str) == -1) + return -1; + return 0; }
@@ -347,8 +351,9 @@ guestfs_close (guestfs_h *g) if (g->test_fp != NULL) fclose (g->test_fp);
- /* Remove temporary directory. */ + /* Remove temporary directories. */ guestfs_int_remove_tmpdir (g); + guestfs_int_remove_sockdir (g);
/* Mark the handle as dead and then free up all memory. */ g->state = NO_HANDLE; @@ -377,7 +382,9 @@ guestfs_close (guestfs_h *g) if (g->pda) hash_free (g->pda); free (g->tmpdir); + free (g->sockdir); free (g->env_tmpdir); + free (g->env_runtimedir); free (g->int_tmpdir); free (g->int_cachedir); free (g->last_error); diff --git a/src/launch.c b/src/launch.c index 60f02a7..9273c58 100644 --- a/src/launch.c +++ b/src/launch.c @@ -425,12 +425,15 @@ int guestfs_int_create_socketname (guestfs_h *g, const char *filename, char (*sockpath)[UNIX_PATH_MAX]) { - if (strlen (g->tmpdir) + 1 + strlen (filename) > UNIX_PATH_MAX-1) { - error (g, _("socket path too long: %s/%s"), g->tmpdir, filename); + if (guestfs_int_lazy_make_sockdir (g) == -1) + return -1; + + if (strlen (g->sockdir) + 1 + strlen (filename) > UNIX_PATH_MAX-1) { + error (g, _("socket path too long: %s/%s"), g->sockdir, filename); return -1; }
- snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", g->tmpdir, filename); + snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", g->sockdir, filename);
return 0; } diff --git a/src/tmpdirs.c b/src/tmpdirs.c index 2ae9c74..e66ab9c 100644 --- a/src/tmpdirs.c +++ b/src/tmpdirs.c @@ -76,6 +76,12 @@ guestfs_int_set_env_tmpdir (guestfs_h *g, const char *tmpdir) }
int +guestfs_int_set_env_runtimedir (guestfs_h *g, const char *runtimedir) +{ + return set_abs_path (g, runtimedir, &g->env_runtimedir); +} + +int guestfs_impl_set_tmpdir (guestfs_h *g, const char *tmpdir) { return set_abs_path (g, tmpdir, &g->int_tmpdir); @@ -119,6 +125,20 @@ guestfs_impl_get_cachedir (guestfs_h *g) return safe_strdup (g, str); }
+/* Note this actually calculates the sockdir, so it never returns NULL. */ +char * +guestfs_impl_get_sockdir (guestfs_h *g) +{ + const char *str; + + if (g->env_runtimedir) + str = g->env_runtimedir; + else + str = "/tmp"; + + return safe_strdup (g, str); +} + static int lazy_make_tmpdir (guestfs_h *g, char *(*getdir) (guestfs_h *g), char **dest) { @@ -145,6 +165,12 @@ guestfs_int_lazy_make_tmpdir (guestfs_h *g) return lazy_make_tmpdir (g, guestfs_get_tmpdir, &g->tmpdir); }
+int +guestfs_int_lazy_make_sockdir (guestfs_h *g) +{ + return lazy_make_tmpdir (g, guestfs_get_sockdir, &g->sockdir); +} + /* Recursively remove a temporary directory. If removal fails, just * return (it's a temporary directory so it'll eventually be cleaned * up by a temp cleaner). This is done using "rm -rf" because that's @@ -167,3 +193,10 @@ guestfs_int_remove_tmpdir (guestfs_h *g) if (g->tmpdir) guestfs_int_recursive_remove_dir (g, g->tmpdir); } + +void +guestfs_int_remove_sockdir (guestfs_h *g) +{ + if (g->sockdir) + guestfs_int_recursive_remove_dir (g, g->sockdir); +} diff --git a/test-tool/test-tool.c b/test-tool/test-tool.c index 9d23d6d..495316b 100644 --- a/test-tool/test-tool.c +++ b/test-tool/test-tool.c @@ -210,6 +210,9 @@ main (int argc, char *argv[]) p = getenv ("PATH"); if (p) printf ("PATH=%s\n", p); + p = getenv ("XDG_RUNTIME_DIR"); + if (p) + printf ("XDG_RUNTIME_DIR=%s\n", p);
/* Print SELinux mode (don't worry if this fails, or if the command * doesn't even exist). @@ -255,6 +258,9 @@ main (int argc, char *argv[]) printf ("guestfs_get_recovery_proc: %d\n", guestfs_get_recovery_proc (g)); printf ("guestfs_get_selinux: %d\n", guestfs_get_selinux (g)); printf ("guestfs_get_smp: %d\n", guestfs_get_smp (g)); + p = guestfs_get_sockdir (g); + printf ("guestfs_get_sockdir: %s\n", p ? : "(null)"); + free (p); p = guestfs_get_tmpdir (g); printf ("guestfs_get_tmpdir: %s\n", p ? : "(null)"); free (p);
ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Wed, Feb 03, 2016 at 01:17:42PM +0100, Pino Toscano wrote:
Introduce a new read-only API to get a path where to store temporary sockets: this is different from tmpdir, as we need short paths for sockets (due to sockaddr_un::sun_path), and it is either XDG_RUNTIME_DIR if set, or /tmp; adapt guestfs_int_create_socketname to create sockets in that location.
Furthermore, print sockdir and XDG_RUNTIME_DIR in test-tool for debugging.
As you saw, there were a few problems with this patch. However I also found something more fundamental. On machines where XDG_RUNTIME_DIR is set to /run/user/$UID, it fails badly when run as root: Original error from libvirt: internal error: process exited while connecting to monitor: 2016-02-08T19:17:42.375986Z qemu-system-x86_64: -chardev socket,id=charserial0,path=/run/user/0/libguestfsdittS9/console.sock: Failed to connect socket: Permission denied [code=1 int1=-1] This is because libvirt runs the appliance as qemu.qemu, which cannot access /run/user/0 (mode 0700). This is the default configuration when accessing a remote machine using `ssh root@remote virt-tool ...' I think we should drop all references to XDG_RUNTIME_DIR (as I noted before, I don't have a high regard for freedesktop pseudo-standards, which I believe are just a way for some people to "policy launder" junk into Linux). The attached patch does that. Note the get-sockdir function now returns the hard-coded value "/tmp", which may or may not be a good idea. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On Monday 08 February 2016 19:34:10 Richard W.M. Jones wrote:
On Wed, Feb 03, 2016 at 01:17:42PM +0100, Pino Toscano wrote:
Introduce a new read-only API to get a path where to store temporary sockets: this is different from tmpdir, as we need short paths for sockets (due to sockaddr_un::sun_path), and it is either XDG_RUNTIME_DIR if set, or /tmp; adapt guestfs_int_create_socketname to create sockets in that location.
Furthermore, print sockdir and XDG_RUNTIME_DIR in test-tool for debugging.
As you saw, there were a few problems with this patch. However I also found something more fundamental.
On machines where XDG_RUNTIME_DIR is set to /run/user/$UID, it fails badly when run as root:
Original error from libvirt: internal error: process exited while connecting to monitor: 2016-02-08T19:17:42.375986Z qemu-system-x86_64: -chardev socket,id=charserial0,path=/run/user/0/libguestfsdittS9/console.sock: Failed to connect socket: Permission denied [code=1 int1=-1]
This is because libvirt runs the appliance as qemu.qemu, which cannot access /run/user/0 (mode 0700).
This is the default configuration when accessing a remote machine using `ssh root@remote virt-tool ...'
This is not due to the work itself, but to the limitations coming from other parties involved (libvirt in this case).
I think we should drop all references to XDG_RUNTIME_DIR (as I noted before, I don't have a high regard for freedesktop pseudo-standards, which I believe are just a way for some people to "policy launder" junk into Linux).
There isn't any needed to scrap everything at the first issue, there actually is a way to make this work better. Also, not having high regards does not automatically mean that things are obnoxious, not that there isn't any middle ground possible.
The attached patch does that. Note the get-sockdir function now returns the hard-coded value "/tmp", which may or may not be a good idea.
NACK for me. Attached there is a (IMHO) better approach to the issue found with libvirt: since only root needs particular changes, then limit them only for this user. After all, we all recommend using libguestfs and its tools as normal user as much as possible, right? So we shouldn't make the common user case worse only for non-recommended use cases. Thanks, -- Pino Toscano
On Tue, Feb 09, 2016 at 11:51:57AM +0100, Pino Toscano wrote:
On Monday 08 February 2016 19:34:10 Richard W.M. Jones wrote:
On Wed, Feb 03, 2016 at 01:17:42PM +0100, Pino Toscano wrote:
Introduce a new read-only API to get a path where to store temporary sockets: this is different from tmpdir, as we need short paths for sockets (due to sockaddr_un::sun_path), and it is either XDG_RUNTIME_DIR if set, or /tmp; adapt guestfs_int_create_socketname to create sockets in that location.
Furthermore, print sockdir and XDG_RUNTIME_DIR in test-tool for debugging.
As you saw, there were a few problems with this patch. However I also found something more fundamental.
On machines where XDG_RUNTIME_DIR is set to /run/user/$UID, it fails badly when run as root:
Original error from libvirt: internal error: process exited while connecting to monitor: 2016-02-08T19:17:42.375986Z qemu-system-x86_64: -chardev socket,id=charserial0,path=/run/user/0/libguestfsdittS9/console.sock: Failed to connect socket: Permission denied [code=1 int1=-1]
This is because libvirt runs the appliance as qemu.qemu, which cannot access /run/user/0 (mode 0700).
This is the default configuration when accessing a remote machine using `ssh root@remote virt-tool ...'
This is not due to the work itself, but to the limitations coming from other parties involved (libvirt in this case).
I think we should drop all references to XDG_RUNTIME_DIR (as I noted before, I don't have a high regard for freedesktop pseudo-standards, which I believe are just a way for some people to "policy launder" junk into Linux).
There isn't any needed to scrap everything at the first issue, there actually is a way to make this work better.
Also, not having high regards does not automatically mean that things are obnoxious, not that there isn't any middle ground possible.
The attached patch does that. Note the get-sockdir function now returns the hard-coded value "/tmp", which may or may not be a good idea.
NACK for me.
Attached there is a (IMHO) better approach to the issue found with libvirt: since only root needs particular changes, then limit them only for this user. After all, we all recommend using libguestfs and its tools as normal user as much as possible, right? So we shouldn't make the common user case worse only for non-recommended use cases.
Thanks, -- Pino Toscano
From 772f649e595d202bdb67f05aeb62157c1104be89 Mon Sep 17 00:00:00 2001 From: Pino Toscano <ptoscano@redhat.com> Date: Tue, 9 Feb 2016 11:36:23 +0100 Subject: [PATCH] lib: fix sockdir for root
When running as root libvirt will launch qemu as qemu.qemu, which will not be able to read inside the socket directory in case it is set as XDG_RUNTIME_DIR under /run/user/0.
Since normal users don't need particular extra access permissions for their sockdirs, restrict /tmp as only possible sockdir for root, changing the permissions only for this user (and making this operation fatal).
Fixes commit 55202a4d49a101392148d79cb2e1591428db2681 and commit 7453952d2456272624f154082e4fc4244af8e507. --- src/launch.c | 6 ------ src/tmpdirs.c | 35 +++++++++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/src/launch.c b/src/launch.c index 9e9960a..9273c58 100644 --- a/src/launch.c +++ b/src/launch.c @@ -428,12 +428,6 @@ guestfs_int_create_socketname (guestfs_h *g, const char *filename, if (guestfs_int_lazy_make_sockdir (g) == -1) return -1;
- /* Allow qemu (which may be running as qemu.qemu) to read the socket - * temporary directory. (RHBZ#610880). - */ - if (chmod (g->sockdir, 0755) == -1) - warning (g, "chmod: %s: %m (ignored)", g->sockdir); - if (strlen (g->sockdir) + 1 + strlen (filename) > UNIX_PATH_MAX-1) { error (g, _("socket path too long: %s/%s"), g->sockdir, filename); return -1; diff --git a/src/tmpdirs.c b/src/tmpdirs.c index e66ab9c..76bf1c5 100644 --- a/src/tmpdirs.c +++ b/src/tmpdirs.c @@ -23,6 +23,7 @@ #include <sys/types.h> #include <sys/stat.h> #include <libintl.h> +#include <unistd.h>
#include "ignore-value.h"
@@ -130,11 +131,20 @@ char * guestfs_impl_get_sockdir (guestfs_h *g) { const char *str; + uid_t euid = geteuid ();
- if (g->env_runtimedir) - str = g->env_runtimedir; - else + if (euid == 0) { + /* Use /tmp exclusively for root, as otherwise qemu (running as + * qemu.qemu when launched by libvirt) will not be able to access + * the directory. + */ str = "/tmp"; + } else { + if (g->env_runtimedir) + str = g->env_runtimedir; + else + str = "/tmp"; + }
return safe_strdup (g, str); } @@ -168,7 +178,24 @@ guestfs_int_lazy_make_tmpdir (guestfs_h *g) int guestfs_int_lazy_make_sockdir (guestfs_h *g) { - return lazy_make_tmpdir (g, guestfs_get_sockdir, &g->sockdir); + int ret; + uid_t euid = geteuid (); + + ret = lazy_make_tmpdir (g, guestfs_get_sockdir, &g->sockdir); + if (ret == -1) + return ret; + + if (euid == 0) { + /* Allow qemu (which may be running as qemu.qemu) to read the socket + * temporary directory. (RHBZ#610880). + */ + if (chmod (g->sockdir, 0755) == -1) { + perrorf (g, "chmod: %s", g->sockdir); + return -1; + } + } + + return ret; }
/* Recursively remove a temporary directory. If removal fails, just
ACK. Since the chmod in this case has been folded into the guestfs_int_lazy_make_sockdir function, there should probably be a followup commit to do the same for guestfs_int_lazy_make_tmpdir. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On Wed, Feb 03, 2016 at 01:17:41PM +0100, Pino Toscano wrote:
Introduce an internal helper to create paths for sockets -- will be useful for changing later the logic for placing sockets. Futhermore, check that the length of sockets won't overflow the buffer for their filenames. --- src/guestfs-internal.h | 1 + src/launch-direct.c | 4 +++- src/launch-libvirt.c | 10 ++++++---- src/launch.c | 17 +++++++++++++++++ 4 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index 5ecd322..bff9f64 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -782,6 +782,7 @@ extern void guestfs_int_launch_send_progress (guestfs_h *g, int perdozen); extern char *guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_dev, int flags); #define APPLIANCE_COMMAND_LINE_IS_TCG 1 const char *guestfs_int_get_cpu_model (int kvm); +int guestfs_int_create_socketname (guestfs_h *g, const char *filename, char (*sockname)[UNIX_PATH_MAX]); extern void guestfs_int_register_backend (const char *name, const struct backend_ops *); extern int guestfs_int_set_backend (guestfs_h *g, const char *method);
diff --git a/src/launch-direct.c b/src/launch-direct.c index b8e453d..a81d4b3 100644 --- a/src/launch-direct.c +++ b/src/launch-direct.c @@ -295,7 +295,9 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) /* Using virtio-serial, we need to create a local Unix domain socket * for qemu to connect to. */ - snprintf (data->guestfsd_sock, sizeof data->guestfsd_sock, "%s/guestfsd.sock", g->tmpdir); + if (guestfs_int_create_socketname (g, "guestfsd.sock", + &data->guestfsd_sock) == -1) + goto cleanup0;
daemon_accept_sock = socket (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0); if (daemon_accept_sock == -1) { diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c index 8a5d93e..376bd80 100644 --- a/src/launch-libvirt.c +++ b/src/launch-libvirt.c @@ -395,8 +395,9 @@ launch_libvirt (guestfs_h *g, void *datav, const char *libvirt_uri) /* Using virtio-serial, we need to create a local Unix domain socket * for qemu to connect to. */ - snprintf (data->guestfsd_path, sizeof data->guestfsd_path, - "%s/guestfsd.sock", g->tmpdir); + if (guestfs_int_create_socketname (g, "guestfsd.sock", + &data->guestfsd_path) == -1) + goto cleanup;
set_socket_create_context (g);
@@ -421,8 +422,9 @@ launch_libvirt (guestfs_h *g, void *datav, const char *libvirt_uri) }
/* For the serial console. */ - snprintf (data->console_path, sizeof data->console_path, - "%s/console.sock", g->tmpdir); + if (guestfs_int_create_socketname (g, "console.sock", + &data->console_path) == -1) + goto cleanup;
console_sock = socket (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0); if (console_sock == -1) { diff --git a/src/launch.c b/src/launch.c index f59818f..60f02a7 100644 --- a/src/launch.c +++ b/src/launch.c @@ -418,6 +418,23 @@ guestfs_int_get_cpu_model (int kvm) #endif }
+/* Create the path for a socket with the selected filename in the + * tmpdir. + */ +int +guestfs_int_create_socketname (guestfs_h *g, const char *filename, + char (*sockpath)[UNIX_PATH_MAX]) +{ + if (strlen (g->tmpdir) + 1 + strlen (filename) > UNIX_PATH_MAX-1) { + error (g, _("socket path too long: %s/%s"), g->tmpdir, filename); + return -1; + } + + snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", g->tmpdir, filename); + + return 0; +} + /* glibc documents, but does not actually implement, a 'getumask(3)' * call. This implements a thread-safe way to get the umask. Note * this is only called when g->verbose is true and after g->tmpdir
Looks good, ACK. 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
participants (2)
-
Pino Toscano -
Richard W.M. Jones