[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] sockets: improve error reporting if UNIX soc
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2] sockets: improve error reporting if UNIX socket path is too long |
Date: |
Wed, 24 May 2017 11:50:02 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 |
On 05/24/2017 11:18 AM, Daniel P. Berrange wrote:
> The 'struct sockaddr_un' only allows 108 bytes for the socket
> path.
>
> If the user supplies a path, QEMU uses snprintf() to silently
> truncate it when too long. This is undesirable because the user
> will then be unable to connect to the path they asked for.
>
> If the user doesn't supply a path, QEMU builds one based on
> TMPDIR, but if that leads to an overlong path, it mistakenly
> uses error_setg_errno() with a stale errno value, because
> snprintf() does not set errno on truncation.
>
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
> util/qemu-sockets.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
> + pathlen = saddr->path ? strlen(saddr->path) : 0;
> + if (pathlen != 0) {
> + if (pathlen > sizeof(un.sun_path)) {
> + error_setg(errp, "UNIX socket path '%s' is too long",
> saddr->path);
> + error_append_hint(errp, "Path must be less than %zu bytes\n",
> + sizeof(un.sun_path));
> + goto err;
> + }
> + strncpy(un.sun_path, saddr->path, sizeof(un.sun_path));
Does NOT nul-terminate if pathlen is exactly 108 - but it looks like
Linux is just fine with a Unix socket name that long.
> } else {
> + const char *template = "qemu-socket-XXXXXX";
> const char *tmpdir = getenv("TMPDIR");
> tmpdir = tmpdir ? tmpdir : "/tmp";
> - if (snprintf(un.sun_path, sizeof(un.sun_path),
> "%s/qemu-socket-XXXXXX",
> - tmpdir) >= sizeof(un.sun_path)) {
> - error_setg_errno(errp, errno,
> - "TMPDIR environment variable (%s) too large",
> tmpdir);
> + if (snprintf(un.sun_path, sizeof(un.sun_path), "%s/%s",
> + tmpdir, template) >= sizeof(un.sun_path)) {
> + error_setg(errp,
> + "TMPDIR environment variable (%s) too large", tmpdir);
> + error_append_hint(errp, "Path must be less than %zu bytes\n",
> + sizeof(un.sun_path) - strlen(template) - 1);
Meanwhile, this branch requires nul-termination (that is, a maximum
generated name of 107), but that's because we then pass un.sun_path to
mkstemp() and g_strdup(), both of which require nul-termination. It's a
bit odd to see the asymmetry between a user-specified name going all the
way to 108 while generated cannot, but not the end of the world.
You do, however, have one more problem to fix:
if (unlink(un.sun_path) < 0 && errno != ENOENT) {
That now needs to be 'if (unlink(saddr->path) ...', because un.sun_path
is no longer guaranteed to be NUL-terminated (although saddr->path is).
Likewise for the two error_setg_errno() calls after that point.
With that fixed,
Reviewed-by: Eric Blake <address@hidden>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature