[Libguestfs] [PATCH 6/6] launch: avoid too long paths for sockets
Richard W.M. Jones
rjones at redhat.com
Fri Jan 29 19:14:58 UTC 2016
On Fri, Jan 29, 2016 at 07:25:30PM +0100, Pino Toscano wrote:
> The API for UNIX sockets limits the path to a static size (usually 104
> or 108 characters, NULL included), which is internally represented as
> UNIX_PATH_MAX. If the temporary directory set is long enough (e.g.
> when running tools as uninstalled, using "run") then these socket paths
> get trucated, if not even cause failures when binding the sockets.
>
> Introduce a new internal API to create paths for sockets, and a new
> helper path where store them in the above case. This new path is created
> only when needed, on a temporary directory under /tmp (hardcoded), and
> cleaned when the handle is destroyed.
I think reading between the lines, you want to hard code /tmp so that
UNIX_PATH_MAX can never be exceeded, even if the user sets $TMPDIR to
some stupid long path?
This new behaviour certainly needs to be documented, eg. in
guestfs(3), because it changes an assumption that setting TMPDIR will
move every temporary file that libguestfs creates.
Are there machines where /tmp is unusable?
Should we provide an API to read the sockname, analogous to
guestfs_get_tmpdir and guestfs_get_cachedir?
Should we use /run or /dev/shm instead? I would say, not /run/user
because systemd doesn't reliably create it, unfortunately, but maybe
somewhere else in /run would be acceptable.
How does libvirt handle socket paths? Does it put them in /run, and
can we emulate its behaviour?
Are there implications for testing? [When running tests, the ./run
script sets TMPDIR to point to /path/to/libguestfs/tmp. Mainly I did
that so that if you ran multiple parallel copies of the tests (eg.
you are testing libguestfs master and a stable branch), you don't want
the appliance to constantly get rebuilt.]
Rich.
> src/guestfs-internal.h | 8 ++++++++
> src/handle.c | 4 +++-
> src/launch-direct.c | 4 +++-
> src/launch-libvirt.c | 10 ++++++----
> src/launch.c | 30 ++++++++++++++++++++++++++++++
> src/tmpdirs.c | 22 ++++++++++++++++++++++
> 6 files changed, 72 insertions(+), 6 deletions(-)
>
> diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
> index 5ecd322..5575aff 100644
> --- a/src/guestfs-internal.h
> +++ b/src/guestfs-internal.h
> @@ -438,6 +438,11 @@ struct guestfs_h
> char *env_tmpdir; /* $TMPDIR (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 */
> + /* Optional socket directory - this is created on demand only when
> + * creating a socket whose absolute path in tmpdir would not fit
> + * into sockaddr_un::sun_path.
> + */
> + char *sockdir;
>
> /* Error handler, plus stack of old error handlers. */
> guestfs_error_handler_cb error_cb;
> @@ -758,7 +763,9 @@ 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_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 */
> @@ -782,6 +789,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/handle.c b/src/handle.c
> index 947818c..076e3fd 100644
> --- a/src/handle.c
> +++ b/src/handle.c
> @@ -347,8 +347,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;
> @@ -380,6 +381,7 @@ guestfs_close (guestfs_h *g)
> free (g->env_tmpdir);
> free (g->int_tmpdir);
> free (g->int_cachedir);
> + free (g->sockdir);
> free (g->last_error);
> free (g->identifier);
> free (g->program);
> 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..1f1f93d 100644
> --- a/src/launch.c
> +++ b/src/launch.c
> @@ -418,6 +418,36 @@ guestfs_int_get_cpu_model (int kvm)
> #endif
> }
>
> +/* Create the path for a socket with the selected filename in the tmpdir,
> + * falling back to creating a separate sockdir and using that as base.
> + */
> +int
> +guestfs_int_create_socketname (guestfs_h *g, const char *filename,
> + char (*sockpath)[UNIX_PATH_MAX])
> +{
> + char *path = g->tmpdir;
> +
> + /* Check whether the new socket can fit into tmpdir. */
> + if (strlen (path) + 1 + strlen (filename) > UNIX_PATH_MAX-1) {
> + if (guestfs_int_lazy_make_sockdir (g) == -1)
> + return -1;
> +
> + path = g->sockdir;
> +
> + /* Safety check for the new path in sockdir. */
> + if (strlen (path) + 1 + strlen (filename) > UNIX_PATH_MAX-1) {
> + error (g, _("cannot get a path for the socket '%s'"),
> + filename);
> + return -1;
> + }
> + }
> +
> + snprintf (*sockpath, UNIX_PATH_MAX-1, "%s/%s", path, filename);
> + (*sockpath)[UNIX_PATH_MAX-1] = '\0';
> +
> + 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
> diff --git a/src/tmpdirs.c b/src/tmpdirs.c
> index 9154d8b..e406e53 100644
> --- a/src/tmpdirs.c
> +++ b/src/tmpdirs.c
> @@ -139,6 +139,21 @@ guestfs_int_lazy_make_tmpdir (guestfs_h *g)
> return 0;
> }
>
> +int
> +guestfs_int_lazy_make_sockdir (guestfs_h *g)
> +{
> + if (!g->sockdir) {
> + g->sockdir = safe_strdup (g, "/tmp/guestfs-sockXXXXXX");
> + if (mkdtemp (g->sockdir) == NULL) {
> + perrorf (g, _("%s: cannot create temporary directory"), g->sockdir);
> + free (g->sockdir);
> + g->sockdir = NULL;
> + return -1;
> + }
> + }
> + return 0;
> +}
> +
> /* 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
> @@ -161,3 +176,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);
> +}
> --
> 2.5.0
>
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
--
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
More information about the Libguestfs
mailing list