[Qemu-devel] [PATCH 0/3] vnc: Fixes for unix socket error handling

[Qemu-devel] [PATCH 0/3] vnc: Fixes for unix socket error handling

[Cole Robinson]

Minor fixes for unix socket error handling, see patches for details

Cole Robinson (3):
  vnc: Don't assert if opening unix socket fails
  vnc: Tweak error when init fails
  qemu-sockets: Report explicit error if unlink fails

 ui/vnc.c            | 5 +++--
 util/qemu-sockets.c | 6 +++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

-- 
2.3.6

[Qemu-devel] [PATCH 1/3] vnc: Don't assert if opening unix socket fails

[Cole Robinson]

Reproducer:

$ qemu-system-x86_64 -display vnc=unix:/root/i-cant-access-you.sock
qemu-system-x86_64: iohandler.c:60: qemu_set_fd_handler2: Assertion `fd >= 0' failed.
Aborted (core dumped)

Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 ui/vnc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ui/vnc.c b/ui/vnc.c
index cffb5b7..f6b36e4 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3685,6 +3685,8 @@ void vnc_display_open(const char *id, Error **errp)
         /* listen for connects */
         if (strncmp(vnc, "unix:", 5) == 0) {
             vs->lsock = unix_listen(vnc+5, NULL, 0, errp);
+            if (vs->lsock < 0)
+                goto fail;
             vs->is_unix = true;
         } else {
             vs->lsock = inet_listen_opts(sopts, 5900, errp);
-- 
2.3.6

Re: [Qemu-devel] [PATCH 1/3] vnc: Don't assert if opening unix socket fails

[Gerd Hoffmann]

On Mi, 2015-04-29 at 12:37 -0400, Cole Robinson wrote:
> +            if (vs->lsock < 0)
> +                goto fail;

fails checkpatch.pl

[Qemu-devel] [PATCH 2/3] vnc: Tweak error when init fails

[Cole Robinson]

Before:
qemu-system-x86_64: -display vnc=unix:/root/foo.sock: Failed to start VNC server on `(null)': Failed to bind socket to /root/foo.sock: Permission denied

After:
qemu-system-x86_64: -display vnc=unix:/root/foo.sock: Failed to start VNC server: Failed to bind socket to /root/foo.sock: Permission denied

Rather than tweak the string possibly show unix: value as well,
just drop the explicit display reporting. We already get the cli
string in the error message, that should be sufficient.

Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 ui/vnc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index f6b36e4..7cba2c8 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3779,8 +3779,7 @@ int vnc_init_func(QemuOpts *opts, void *opaque)
     vnc_display_init(id);
     vnc_display_open(id, &local_err);
     if (local_err != NULL) {
-        error_report("Failed to start VNC server on `%s': %s",
-                     qemu_opt_get(opts, "display"),
+        error_report("Failed to start VNC server: %s",
                      error_get_pretty(local_err));
         error_free(local_err);
         exit(1);
-- 
2.3.6

[Qemu-devel] [PATCH 3/3] qemu-sockets: Report explicit error if unlink fails

[Cole Robinson]

It's possible via libvirt for a user to request an explicit socket path
that qemu lacks permissions to unlink, but the error that's reported
is from bind(2), 'Address already in use'

bind(2) will fail if the unix socket path already exists, so we need to
unlink first. But we should report the unlink failure

Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 util/qemu-sockets.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 87c9bc6..9ef3f7c 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -729,7 +729,11 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
         qemu_opt_set(opts, "path", un.sun_path, &error_abort);
     }
 
-    unlink(un.sun_path);
+    if (access(un.sun_path, F_OK) &&
+        unlink(un.sun_path) < 0) {
+        error_setg_errno(errp, errno, "Failed to unlink socket %s", un.sun_path);
+        goto err;
+    }
     if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
         error_setg_errno(errp, errno, "Failed to bind socket to %s", un.sun_path);
         goto err;
-- 
2.3.6

[Qemu-devel] [PATCH v2] qemu-sockets: Report explicit error if unlink fails

[Cole Robinson]

Consider this case:

$ ls -ld ~/root-owned/
drwx--x--x. 2 root root 4096 Apr 29 12:55 /home/crobinso/root-owned/
$ ls -l ~/root-owned/foo.sock
-rwxrwxrwx. 1 crobinso crobinso 0 Apr 29 12:55 /home/crobinso/root-owned/foo.sock

$ qemu-system-x86_64 -vnc unix:~/root-owned/foo.sock
qemu-system-x86_64: -vnc unix:/home/crobinso/root-owned/foo.sock: Failed to start VNC server: Failed to bind socket to /home/crobinso/root-owned/foo.sock: Address already in use

...which is techinically true, but the real error is that we failed to
unlink. So report it.

This may seem pathological but it's a real possibility via libvirt.

Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
Sigh, sent an old version of the patch accidentally

v2:
    Fix the access check
    Better commit message

 util/qemu-sockets.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 87c9bc6..527f488 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -729,7 +729,11 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
         qemu_opt_set(opts, "path", un.sun_path, &error_abort);
     }
 
-    unlink(un.sun_path);
+    if ((access(un.sun_path, F_OK) == 0) &&
+        unlink(un.sun_path) < 0) {
+        error_setg_errno(errp, errno, "Failed to unlink socket %s", un.sun_path);
+        goto err;
+    }
     if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
         error_setg_errno(errp, errno, "Failed to bind socket to %s", un.sun_path);
         goto err;
-- 
2.3.6

Re: [Qemu-devel] [PATCH v2] qemu-sockets: Report explicit error if unlink fails

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

On 04/29/2015 11:03 AM, Cole Robinson wrote:
> Consider this case:
> 
> $ ls -ld ~/root-owned/
> drwx--x--x. 2 root root 4096 Apr 29 12:55 /home/crobinso/root-owned/
> $ ls -l ~/root-owned/foo.sock
> -rwxrwxrwx. 1 crobinso crobinso 0 Apr 29 12:55 /home/crobinso/root-owned/foo.sock
> 
> $ qemu-system-x86_64 -vnc unix:~/root-owned/foo.sock
> qemu-system-x86_64: -vnc unix:/home/crobinso/root-owned/foo.sock: Failed to start VNC server: Failed to bind socket to /home/crobinso/root-owned/foo.sock: Address already in use
> 
> ...which is techinically true, but the real error is that we failed to
> unlink. So report it.
> 
> This may seem pathological but it's a real possibility via libvirt.
> 
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
> Sigh, sent an old version of the patch accidentally
> 
> v2:
>     Fix the access check
>     Better commit message

Sigh, and I was too hasty in my review of v1 to notice the logic bug in
access() use.  v2 is definitely better:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/3] vnc: Fixes for unix socket error handling

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 448 bytes --]

On 04/29/2015 10:37 AM, Cole Robinson wrote:
> Minor fixes for unix socket error handling, see patches for details
> 
> Cole Robinson (3):
>   vnc: Don't assert if opening unix socket fails
>   vnc: Tweak error when init fails
>   qemu-sockets: Report explicit error if unlink fails

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]