vnc: Fixes for unix socket error handling

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

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)

---
 ui/vnc.c | 2 ++
 1 file changed, 2 insertions(+)

index cffb5b7..f6b36e4 100644
@@ -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

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.

---
 ui/vnc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index f6b36e4..7cba2c8 100644
@@ -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

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

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

index 87c9bc6..9ef3f7c 100644
@@ -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

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.

---
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(-)

index 87c9bc6..527f488 100644
@@ -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

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

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