[Qemu-devel] [PATCH v3 0/4] io: Various fixes around QIOChannel Features

[Qemu-devel] [PATCH v3 0/4] io: Various fixes around QIOChannel Features

[Felipe Franciosi]

This series include four patches around the utilisation of QIOChannel
features. The first patch actually fixes a bug, while the next two
makes the test/set of features consistent by using helper functions.
The last patch adds a test to verify that the bug has been fixed.

Changes from v2:
 - Add a test which fails on master but passes with these fixes
 - Rebase on latest master

Changes from v1:
 - Fix two pointer declarations (were missing a star)
 - Fix a call to _set_features() which should be _set_feature()

Felipe Franciosi (4):
  io: Fix double shift usages on QIOChannel features
  io: Use qio_channel_has_feature() where applicable
  io: Introduce a qio_channel_set_feature() helper
  io: Add a QIOChannelSocket cleanup test

 include/io/channel.h           |   16 +++++++++++++---
 io/channel-socket.c            |   12 +++++++-----
 io/channel-tls.c               |    4 ++--
 io/channel-websock.c           |    4 ++--
 io/channel.c                   |   11 +++++++++--
 tests/test-io-channel-socket.c |   33 +++++++++++++++++++++++++++++++++
 6 files changed, 66 insertions(+), 14 deletions(-)

[Qemu-devel] [PATCH v3 1/4] io: Fix double shift usages on QIOChannel features

[Felipe Franciosi]

When QIOChannels were introduced in 666a3af9, the feature bits were
already defined shifted. However, when using them, the code was shifting
them again. The incorrect use was consistent until 74b6ce43, where
QIO_CHANNEL_FEATURE_LISTEN was defined shifted but tested unshifted.

This patch changes the definition to be unshifted and fixes the
incorrect usage introduced on 74b6ce43.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 include/io/channel.h |    6 +++---
 io/channel-socket.c  |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/io/channel.h b/include/io/channel.h
index 752e89f..5368604 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -40,9 +40,9 @@ typedef struct QIOChannelClass QIOChannelClass;
 typedef enum QIOChannelFeature QIOChannelFeature;
 
 enum QIOChannelFeature {
-    QIO_CHANNEL_FEATURE_FD_PASS  = (1 << 0),
-    QIO_CHANNEL_FEATURE_SHUTDOWN = (1 << 1),
-    QIO_CHANNEL_FEATURE_LISTEN   = (1 << 2),
+    QIO_CHANNEL_FEATURE_FD_PASS,
+    QIO_CHANNEL_FEATURE_SHUTDOWN,
+    QIO_CHANNEL_FEATURE_LISTEN,
 };
 
 
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 196a4f1..6710b2e 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -403,7 +403,7 @@ static void qio_channel_socket_finalize(Object *obj)
     QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
 
     if (ioc->fd != -1) {
-        if (QIO_CHANNEL(ioc)->features & QIO_CHANNEL_FEATURE_LISTEN) {
+        if (QIO_CHANNEL(ioc)->features & (1 << QIO_CHANNEL_FEATURE_LISTEN)) {
             Error *err = NULL;
 
             socket_listen_cleanup(ioc->fd, &err);
-- 
1.7.1

[Qemu-devel] [PATCH v3 2/4] io: Use qio_channel_has_feature() where applicable

[Felipe Franciosi]

Parts of the code have been testing QIOChannel features directly with a
logical AND. This patch makes it all consistent by using the
qio_channel_has_feature() function to test if a feature is present.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 io/channel-socket.c  |    3 ++-
 io/channel-tls.c     |    2 +-
 io/channel-websock.c |    2 +-
 io/channel.c         |    4 ++--
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index 6710b2e..8fc6e5a 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -403,7 +403,8 @@ static void qio_channel_socket_finalize(Object *obj)
     QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
 
     if (ioc->fd != -1) {
-        if (QIO_CHANNEL(ioc)->features & (1 << QIO_CHANNEL_FEATURE_LISTEN)) {
+        QIOChannel *ioc_local = QIO_CHANNEL(ioc);
+        if (qio_channel_has_feature(ioc_local, QIO_CHANNEL_FEATURE_LISTEN)) {
             Error *err = NULL;
 
             socket_listen_cleanup(ioc->fd, &err);
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 9a8525c..f7bb0e3 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -111,7 +111,7 @@ qio_channel_tls_new_client(QIOChannel *master,
     ioc = QIO_CHANNEL(tioc);
 
     tioc->master = master;
-    if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) {
+    if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
         ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN);
     }
     object_ref(OBJECT(master));
diff --git a/io/channel-websock.c b/io/channel-websock.c
index 533bd4b..75df03e 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -497,7 +497,7 @@ qio_channel_websock_new_server(QIOChannel *master)
     ioc = QIO_CHANNEL(wioc);
 
     wioc->master = master;
-    if (master->features & (1 << QIO_CHANNEL_FEATURE_SHUTDOWN)) {
+    if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
         ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN);
     }
     object_ref(OBJECT(master));
diff --git a/io/channel.c b/io/channel.c
index 923c465..e50325c 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -40,7 +40,7 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc,
     QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
 
     if ((fds || nfds) &&
-        !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) {
+        !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) {
         error_setg_errno(errp, EINVAL,
                          "Channel does not support file descriptor passing");
         return -1;
@@ -60,7 +60,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
     QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
 
     if ((fds || nfds) &&
-        !(ioc->features & (1 << QIO_CHANNEL_FEATURE_FD_PASS))) {
+        !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS)) {
         error_setg_errno(errp, EINVAL,
                          "Channel does not support file descriptor passing");
         return -1;
-- 
1.7.1

[Qemu-devel] [PATCH v3 3/4] io: Introduce a qio_channel_set_feature() helper

[Felipe Franciosi]

Testing QIOChannel feature support can be done with a helper called
qio_channel_has_feature(). Setting feature support, however, was
done manually with a logical OR. This patch introduces a new helper
called qio_channel_set_feature() and makes use of it where applicable.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 include/io/channel.h |   10 ++++++++++
 io/channel-socket.c  |    9 +++++----
 io/channel-tls.c     |    2 +-
 io/channel-websock.c |    2 +-
 io/channel.c         |    7 +++++++
 5 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/include/io/channel.h b/include/io/channel.h
index 5368604..cf1c622 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -149,6 +149,16 @@ bool qio_channel_has_feature(QIOChannel *ioc,
                              QIOChannelFeature feature);
 
 /**
+ * qio_channel_set_feature:
+ * @ioc: the channel object
+ * @feature: the feature to set support for
+ *
+ * Add channel support for the feature named in @feature.
+ */
+void qio_channel_set_feature(QIOChannel *ioc,
+                             QIOChannelFeature feature);
+
+/**
  * qio_channel_readv_full:
  * @ioc: the channel object
  * @iov: the array of memory regions to read data into
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 8fc6e5a..75cbca3 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -55,7 +55,7 @@ qio_channel_socket_new(void)
     sioc->fd = -1;
 
     ioc = QIO_CHANNEL(sioc);
-    ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN);
+    qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
 
 #ifdef WIN32
     ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL);
@@ -107,12 +107,12 @@ qio_channel_socket_set_fd(QIOChannelSocket *sioc,
 #ifndef WIN32
     if (sioc->localAddr.ss_family == AF_UNIX) {
         QIOChannel *ioc = QIO_CHANNEL(sioc);
-        ioc->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS);
+        qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_FD_PASS);
     }
 #endif /* WIN32 */
     if (getsockopt(fd, SOL_SOCKET, SO_ACCEPTCONN, &val, &len) == 0 && val) {
         QIOChannel *ioc = QIO_CHANNEL(sioc);
-        ioc->features |= (1 << QIO_CHANNEL_FEATURE_LISTEN);
+        qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN);
     }
 
     return 0;
@@ -380,7 +380,8 @@ qio_channel_socket_accept(QIOChannelSocket *ioc,
 
 #ifndef WIN32
     if (cioc->localAddr.ss_family == AF_UNIX) {
-        QIO_CHANNEL(cioc)->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS);
+        QIOChannel *ioc_local = QIO_CHANNEL(cioc);
+        qio_channel_set_feature(ioc_local, QIO_CHANNEL_FEATURE_FD_PASS);
     }
 #endif /* WIN32 */
 
diff --git a/io/channel-tls.c b/io/channel-tls.c
index f7bb0e3..d24dc8c 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -112,7 +112,7 @@ qio_channel_tls_new_client(QIOChannel *master,
 
     tioc->master = master;
     if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
-        ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN);
+        qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
     }
     object_ref(OBJECT(master));
 
diff --git a/io/channel-websock.c b/io/channel-websock.c
index 75df03e..f45bced 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -498,7 +498,7 @@ qio_channel_websock_new_server(QIOChannel *master)
 
     wioc->master = master;
     if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
-        ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN);
+        qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
     }
     object_ref(OBJECT(master));
 
diff --git a/io/channel.c b/io/channel.c
index e50325c..d1f1ae5 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -30,6 +30,13 @@ bool qio_channel_has_feature(QIOChannel *ioc,
 }
 
 
+void qio_channel_set_feature(QIOChannel *ioc,
+                             QIOChannelFeature feature)
+{
+    ioc->features |= (1 << feature);
+}
+
+
 ssize_t qio_channel_readv_full(QIOChannel *ioc,
                                const struct iovec *iov,
                                size_t niov,
-- 
1.7.1

[Qemu-devel] [PATCH v3 4/4] io: Add a QIOChannelSocket cleanup test

[Felipe Franciosi]

This patch adds a test to verify that the QIOChannel framework will not
unlink a filesystem unix socket unless the _FEATURE_LISTEN bit is set.

Due to a bug introduced in 74b6ce43, the framework would unlink the
entry if the _FEATURE_SHUTDOWN bit was set, regardless of the presence
of _FEATURE_LISTEN.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 tests/test-io-channel-socket.c |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
index f73e063..aa88c3c 100644
--- a/tests/test-io-channel-socket.c
+++ b/tests/test-io-channel-socket.c
@@ -491,6 +491,37 @@ static void test_io_channel_unix_fd_pass(void)
     }
     g_free(fdrecv);
 }
+
+static void test_io_channel_unix_listen_cleanup(void)
+{
+    QIOChannelSocket *ioc;
+    struct sockaddr_un un;
+    int sock;
+
+#define TEST_SOCKET "test-io-channel-socket.sock"
+
+    ioc = qio_channel_socket_new();
+
+    /* Manually bind ioc without calling the qio api to avoid setting
+     * the LISTEN feature */
+    sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
+    memset(&un, 0, sizeof(un));
+    un.sun_family = AF_UNIX;
+    snprintf(un.sun_path, sizeof(un.sun_path), "%s", TEST_SOCKET);
+    unlink(TEST_SOCKET);
+    bind(sock, (struct sockaddr *)&un, sizeof(un));
+    ioc->fd = sock;
+    ioc->localAddrLen = sizeof(ioc->localAddr);
+    getsockname(sock, (struct sockaddr *)&ioc->localAddr,
+                &ioc->localAddrLen);
+
+    g_assert(g_file_test(TEST_SOCKET, G_FILE_TEST_EXISTS));
+    object_unref(OBJECT(ioc));
+    g_assert(g_file_test(TEST_SOCKET, G_FILE_TEST_EXISTS));
+
+    unlink(TEST_SOCKET);
+}
+
 #endif /* _WIN32 */
 
 
@@ -562,6 +593,8 @@ int main(int argc, char **argv)
                     test_io_channel_unix_async);
     g_test_add_func("/io/channel/socket/unix-fd-pass",
                     test_io_channel_unix_fd_pass);
+    g_test_add_func("/io/channel/socket/unix-listen-cleanup",
+                    test_io_channel_unix_listen_cleanup);
 #endif /* _WIN32 */
 
     return g_test_run();
-- 
1.7.1

Re: [Qemu-devel] [PATCH v3 0/4] io: Various fixes around QIOChannel Features

[Daniel P. Berrange]

On Thu, Sep 29, 2016 at 08:52:34AM -0700, Felipe Franciosi wrote:
> This series include four patches around the utilisation of QIOChannel
> features. The first patch actually fixes a bug, while the next two
> makes the test/set of features consistent by using helper functions.
> The last patch adds a test to verify that the bug has been fixed.

Thanks, this all looks good, and I've queued it for my
next pull request.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH v3 0/4] io: Various fixes around QIOChannel Features

[Felipe Franciosi]

> On 29 Sep 2016, at 17:32, Daniel P. Berrange <berrange@redhat.com> wrote:
> 
> On Thu, Sep 29, 2016 at 08:52:34AM -0700, Felipe Franciosi wrote:
>> This series include four patches around the utilisation of QIOChannel
>> features. The first patch actually fixes a bug, while the next two
>> makes the test/set of features consistent by using helper functions.
>> The last patch adds a test to verify that the bug has been fixed.
> 
> Thanks, this all looks good, and I've queued it for my
> next pull request.

For clarification, the bug introduced in 74b6ce43 happens as follows:

On instance_finalize(), any socket with _FEATURE_LISTEN should be cleaned up. Instead, sockets with _FEATURE_SHUTDOWN end up being cleaned up. All sockets with _LISTEN also have _SHUTDOWN, so it luckily works as intended.

However, sockets with _SHUTDOWN that do not have _LISTEN also get cleaned up (and they shouldn't). The issue there is that unix sockets trigger a call to unlink() with garbage (an uninitialised field in SocketAddress). Since the unlink()'s error code is ENOENT, no one notices (or log) the error.

I couldn't work out a way of exploiting this. Random unlink() calls sound serious enough, though. Perhaps you should consider Marc's recommendation and also pull it to -stable.

Thanks,
Felipe

> 
> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|