[Qemu-devel] [PULL v1 0/3] Misc I/O channel fixes

[Qemu-devel] [PULL v1 0/3] Misc I/O channel fixes

[Daniel P. Berrange]

The following changes since commit 5dc42c186d63b7b338594fc071cf290805dcc5a5:

  Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2015-12-22 14:21:42 +0000)

are available in the git repository at:

  git://github.com/berrange/qemu tags/pull-io-fixes-2015-12-23-1

for you to fetch changes up to 7b3c618ad0cd0154993b5b5dbd34e0010960585a:

  io: fix stack allocation when sending of file descriptors (2015-12-23 10:53:03 +0000)

----------------------------------------------------------------
Merge misc I/O channel fixes

----------------------------------------------------------------
Daniel P. Berrange (3):
      io: bind to loopback IP addrs in test suite
      io: fix setting of QIO_CHANNEL_FEATURE_FD_PASS on server connections
      io: fix stack allocation when sending of file descriptors

 io/channel-socket.c            |  17 ++++--
 tests/test-io-channel-socket.c | 129 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 134 insertions(+), 12 deletions(-)
-- 
2.5.0

[Qemu-devel] [PULL v1 1/3] io: bind to loopback IP addrs in test suite

[Daniel P. Berrange]

The test suite currently binds to 0.0.0.0 or ::, which covers
all interfaces of the machine. It is bad practice for test
suite to open publically accessible ports on a machine, so
switch to use loopback addrs 127.0.0.1 or ::1.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 tests/test-io-channel-socket.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
index 194d043..e76d54c 100644
--- a/tests/test-io-channel-socket.c
+++ b/tests/test-io-channel-socket.c
@@ -261,7 +261,7 @@ static void test_io_channel_ipv4(bool async)
 
     listen_addr->type = SOCKET_ADDRESS_KIND_INET;
     listen_addr->u.inet = g_new0(InetSocketAddress, 1);
-    listen_addr->u.inet->host = g_strdup("0.0.0.0");
+    listen_addr->u.inet->host = g_strdup("127.0.0.1");
     listen_addr->u.inet->port = NULL; /* Auto-select */
 
     connect_addr->type = SOCKET_ADDRESS_KIND_INET;
@@ -295,7 +295,7 @@ static void test_io_channel_ipv6(bool async)
 
     listen_addr->type = SOCKET_ADDRESS_KIND_INET;
     listen_addr->u.inet = g_new0(InetSocketAddress, 1);
-    listen_addr->u.inet->host = g_strdup("::");
+    listen_addr->u.inet->host = g_strdup("::1");
     listen_addr->u.inet->port = NULL; /* Auto-select */
 
     connect_addr->type = SOCKET_ADDRESS_KIND_INET;
-- 
2.5.0

[Qemu-devel] [PULL v1 2/3] io: fix setting of QIO_CHANNEL_FEATURE_FD_PASS on server connections

[Daniel P. Berrange]

The QIO_CHANNEL_FEATURE_FD_PASS feature flag is set in the
qio_channel_socket_set_fd() method, however, this only deals
with client side connections.

To ensure server side connections also have the feature flag
set, we must set it in qio_channel_socket_accept() too. This
also highlighted a typo fix where the code updated the
sockaddr struct in the wrong object instance.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-socket.c            | 10 ++++++++--
 tests/test-io-channel-socket.c | 29 +++++++++++++++++++++++++----
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index 90b3c73..eed2ff5 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -352,13 +352,19 @@ qio_channel_socket_accept(QIOChannelSocket *ioc,
         goto error;
     }
 
-    if (getsockname(cioc->fd, (struct sockaddr *)&ioc->localAddr,
-                    &ioc->localAddrLen) < 0) {
+    if (getsockname(cioc->fd, (struct sockaddr *)&cioc->localAddr,
+                    &cioc->localAddrLen) < 0) {
         error_setg_errno(errp, socket_error(),
                          "Unable to query local socket address");
         goto error;
     }
 
+#ifndef WIN32
+    if (cioc->localAddr.ss_family == AF_UNIX) {
+        QIO_CHANNEL(cioc)->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS);
+    }
+#endif /* WIN32 */
+
     trace_qio_channel_socket_accept_complete(ioc, cioc, cioc->fd);
     return cioc;
 
diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
index e76d54c..b0eb538 100644
--- a/tests/test-io-channel-socket.c
+++ b/tests/test-io-channel-socket.c
@@ -210,13 +210,19 @@ static void test_io_channel_setup_async(SocketAddress *listen_addr,
 
 static void test_io_channel(bool async,
                             SocketAddress *listen_addr,
-                            SocketAddress *connect_addr)
+                            SocketAddress *connect_addr,
+                            bool passFD)
 {
     QIOChannel *src, *dst;
     QIOChannelTest *test;
     if (async) {
         test_io_channel_setup_async(listen_addr, connect_addr, &src, &dst);
 
+        g_assert(!passFD ||
+                 qio_channel_has_feature(src, QIO_CHANNEL_FEATURE_FD_PASS));
+        g_assert(!passFD ||
+                 qio_channel_has_feature(dst, QIO_CHANNEL_FEATURE_FD_PASS));
+
         test = qio_channel_test_new();
         qio_channel_test_run_threads(test, true, src, dst);
         qio_channel_test_validate(test);
@@ -226,6 +232,11 @@ static void test_io_channel(bool async,
 
         test_io_channel_setup_async(listen_addr, connect_addr, &src, &dst);
 
+        g_assert(!passFD ||
+                 qio_channel_has_feature(src, QIO_CHANNEL_FEATURE_FD_PASS));
+        g_assert(!passFD ||
+                 qio_channel_has_feature(dst, QIO_CHANNEL_FEATURE_FD_PASS));
+
         test = qio_channel_test_new();
         qio_channel_test_run_threads(test, false, src, dst);
         qio_channel_test_validate(test);
@@ -235,6 +246,11 @@ static void test_io_channel(bool async,
     } else {
         test_io_channel_setup_sync(listen_addr, connect_addr, &src, &dst);
 
+        g_assert(!passFD ||
+                 qio_channel_has_feature(src, QIO_CHANNEL_FEATURE_FD_PASS));
+        g_assert(!passFD ||
+                 qio_channel_has_feature(dst, QIO_CHANNEL_FEATURE_FD_PASS));
+
         test = qio_channel_test_new();
         qio_channel_test_run_threads(test, true, src, dst);
         qio_channel_test_validate(test);
@@ -244,6 +260,11 @@ static void test_io_channel(bool async,
 
         test_io_channel_setup_sync(listen_addr, connect_addr, &src, &dst);
 
+        g_assert(!passFD ||
+                 qio_channel_has_feature(src, QIO_CHANNEL_FEATURE_FD_PASS));
+        g_assert(!passFD ||
+                 qio_channel_has_feature(dst, QIO_CHANNEL_FEATURE_FD_PASS));
+
         test = qio_channel_test_new();
         qio_channel_test_run_threads(test, false, src, dst);
         qio_channel_test_validate(test);
@@ -269,7 +290,7 @@ static void test_io_channel_ipv4(bool async)
     connect_addr->u.inet->host = g_strdup("127.0.0.1");
     connect_addr->u.inet->port = NULL; /* Filled in later */
 
-    test_io_channel(async, listen_addr, connect_addr);
+    test_io_channel(async, listen_addr, connect_addr, false);
 
     qapi_free_SocketAddress(listen_addr);
     qapi_free_SocketAddress(connect_addr);
@@ -303,7 +324,7 @@ static void test_io_channel_ipv6(bool async)
     connect_addr->u.inet->host = g_strdup("::1");
     connect_addr->u.inet->port = NULL; /* Filled in later */
 
-    test_io_channel(async, listen_addr, connect_addr);
+    test_io_channel(async, listen_addr, connect_addr, false);
 
     qapi_free_SocketAddress(listen_addr);
     qapi_free_SocketAddress(connect_addr);
@@ -337,7 +358,7 @@ static void test_io_channel_unix(bool async)
     connect_addr->u.q_unix = g_new0(UnixSocketAddress, 1);
     connect_addr->u.q_unix->path = g_strdup(TEST_SOCKET);
 
-    test_io_channel(async, listen_addr, connect_addr);
+    test_io_channel(async, listen_addr, connect_addr, true);
 
     qapi_free_SocketAddress(listen_addr);
     qapi_free_SocketAddress(connect_addr);
-- 
2.5.0

[Qemu-devel] [PULL v1 3/3] io: fix stack allocation when sending of file descriptors

[Daniel P. Berrange]

When sending file descriptors over a socket, we have to
allocate a data buffer to hold the FDs in the scmsghdr.
Unfortunately we allocated the buffer on the stack inside
an if () {} block, but called sendmsg() outside the block.
So the stack bytes holding the FDs were liable to be
overwritten with other data. By luck this was not a problem
when sending 1 FD, but if sending 2 or more then it would
fail.

The fix is to simply move the variables outside the nested
'if' block. To keep valgrind quiet we also zero-initialize
the 'control' buffer.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-socket.c            |  7 ++-
 tests/test-io-channel-socket.c | 96 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+), 4 deletions(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index eed2ff5..10a5b31 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -493,15 +493,14 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
     QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
     ssize_t ret;
     struct msghdr msg = { NULL, };
+    char control[CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)] = { 0 };
+    size_t fdsize = sizeof(int) * nfds;
+    struct cmsghdr *cmsg;
 
     msg.msg_iov = (struct iovec *)iov;
     msg.msg_iovlen = niov;
 
     if (nfds) {
-        char control[CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)];
-        size_t fdsize = sizeof(int) * nfds;
-        struct cmsghdr *cmsg;
-
         if (nfds > SOCKET_MAX_FDS) {
             error_setg_errno(errp, -EINVAL,
                              "Only %d FDs can be sent, got %zu",
diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
index b0eb538..1a36a3c 100644
--- a/tests/test-io-channel-socket.c
+++ b/tests/test-io-channel-socket.c
@@ -376,6 +376,100 @@ static void test_io_channel_unix_async(void)
 {
     return test_io_channel_unix(true);
 }
+
+static void test_io_channel_unix_fd_pass(void)
+{
+    SocketAddress *listen_addr = g_new0(SocketAddress, 1);
+    SocketAddress *connect_addr = g_new0(SocketAddress, 1);
+    QIOChannel *src, *dst;
+    int testfd;
+    int fdsend[3];
+    int *fdrecv = NULL;
+    size_t nfdrecv = 0;
+    size_t i;
+    char bufsend[12], bufrecv[12];
+    struct iovec iosend[1], iorecv[1];
+
+#define TEST_SOCKET "test-io-channel-socket.sock"
+#define TEST_FILE "test-io-channel-socket.txt"
+
+    testfd = open(TEST_FILE, O_RDWR|O_TRUNC|O_CREAT, 0700);
+    g_assert(testfd != -1);
+    fdsend[0] = testfd;
+    fdsend[1] = testfd;
+    fdsend[2] = testfd;
+
+    listen_addr->type = SOCKET_ADDRESS_KIND_UNIX;
+    listen_addr->u.q_unix = g_new0(UnixSocketAddress, 1);
+    listen_addr->u.q_unix->path = g_strdup(TEST_SOCKET);
+
+    connect_addr->type = SOCKET_ADDRESS_KIND_UNIX;
+    connect_addr->u.q_unix = g_new0(UnixSocketAddress, 1);
+    connect_addr->u.q_unix->path = g_strdup(TEST_SOCKET);
+
+    test_io_channel_setup_sync(listen_addr, connect_addr, &src, &dst);
+
+    memcpy(bufsend, "Hello World", G_N_ELEMENTS(bufsend));
+
+    iosend[0].iov_base = bufsend;
+    iosend[0].iov_len = G_N_ELEMENTS(bufsend);
+
+    iorecv[0].iov_base = bufrecv;
+    iorecv[0].iov_len = G_N_ELEMENTS(bufrecv);
+
+    g_assert(qio_channel_has_feature(src, QIO_CHANNEL_FEATURE_FD_PASS));
+    g_assert(qio_channel_has_feature(dst, QIO_CHANNEL_FEATURE_FD_PASS));
+
+    qio_channel_writev_full(src,
+                            iosend,
+                            G_N_ELEMENTS(iosend),
+                            fdsend,
+                            G_N_ELEMENTS(fdsend),
+                            &error_abort);
+
+    qio_channel_readv_full(dst,
+                           iorecv,
+                           G_N_ELEMENTS(iorecv),
+                           &fdrecv,
+                           &nfdrecv,
+                           &error_abort);
+
+    g_assert(nfdrecv == G_N_ELEMENTS(fdsend));
+    /* Each recvd FD should be different from sent FD */
+    for (i = 0; i < nfdrecv; i++) {
+        g_assert_cmpint(fdrecv[i], !=, testfd);
+    }
+    /* Each recvd FD should be different from each other */
+    g_assert_cmpint(fdrecv[0], !=, fdrecv[1]);
+    g_assert_cmpint(fdrecv[0], !=, fdrecv[2]);
+    g_assert_cmpint(fdrecv[1], !=, fdrecv[2]);
+
+    /* Check the I/O buf we sent at the same time matches */
+    g_assert(memcmp(bufsend, bufrecv, G_N_ELEMENTS(bufsend)) == 0);
+
+    /* Write some data into the FD we received */
+    g_assert(write(fdrecv[0], bufsend, G_N_ELEMENTS(bufsend)) ==
+             G_N_ELEMENTS(bufsend));
+
+    /* Read data from the original FD and make sure it matches */
+    memset(bufrecv, 0, G_N_ELEMENTS(bufrecv));
+    g_assert(lseek(testfd, 0, SEEK_SET) == 0);
+    g_assert(read(testfd, bufrecv, G_N_ELEMENTS(bufrecv)) ==
+             G_N_ELEMENTS(bufrecv));
+    g_assert(memcmp(bufsend, bufrecv, G_N_ELEMENTS(bufsend)) == 0);
+
+    object_unref(OBJECT(src));
+    object_unref(OBJECT(dst));
+    qapi_free_SocketAddress(listen_addr);
+    qapi_free_SocketAddress(connect_addr);
+    unlink(TEST_SOCKET);
+    unlink(TEST_FILE);
+    close(testfd);
+    for (i = 0; i < nfdrecv; i++) {
+        close(fdrecv[i]);
+    }
+    g_free(fdrecv);
+}
 #endif /* _WIN32 */
 
 
@@ -414,6 +508,8 @@ int main(int argc, char **argv)
                     test_io_channel_unix_sync);
     g_test_add_func("/io/channel/socket/unix-async",
                     test_io_channel_unix_async);
+    g_test_add_func("/io/channel/socket/unix-fd-pass",
+                    test_io_channel_unix_fd_pass);
 #endif /* _WIN32 */
 
     return g_test_run();
-- 
2.5.0

Re: [Qemu-devel] [PULL v1 0/3] Misc I/O channel fixes

[Peter Maydell]

On 23 December 2015 at 10:57, Daniel P. Berrange <berrange@redhat.com> wrote:
> The following changes since commit 5dc42c186d63b7b338594fc071cf290805dcc5a5:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2015-12-22 14:21:42 +0000)
>
> are available in the git repository at:
>
>   git://github.com/berrange/qemu tags/pull-io-fixes-2015-12-23-1
>
> for you to fetch changes up to 7b3c618ad0cd0154993b5b5dbd34e0010960585a:
>
>   io: fix stack allocation when sending of file descriptors (2015-12-23 10:53:03 +0000)
>
> ----------------------------------------------------------------
> Merge misc I/O channel fixes
>
> ----------------------------------------------------------------
> Daniel P. Berrange (3):
>       io: bind to loopback IP addrs in test suite
>       io: fix setting of QIO_CHANNEL_FEATURE_FD_PASS on server connections
>       io: fix stack allocation when sending of file descriptors
>
>  io/channel-socket.c            |  17 ++++--
>  tests/test-io-channel-socket.c | 129 +++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 134 insertions(+), 12 deletions(-)
> --
> 2.5.0
>

Applied, thanks.

-- PMM