[Qemu-devel] [PATCH 0/2] Fixes to FD passing with QIOChannel

[Qemu-devel] [PATCH 0/2] Fixes to FD passing with QIOChannel

[Daniel P. Berrange]

Two small fixes for bugs with FD passing with QIOChannel
discovered indirectly while testing the chardev unit tests.
Added further unit tests to directly cover the FD passing
code.

Daniel P. Berrange (2):
  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 | 127 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 134 insertions(+), 10 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/2] 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.

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 194d043..44b5de7 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

Re: [Qemu-devel] [PATCH 1/2] io: fix setting of QIO_CHANNEL_FEATURE_FD_PASS on server connections

[Eric Blake]

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

On 12/21/2015 09:23 AM, Daniel P. Berrange wrote:
> 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.
> 
> 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) {

Looks like a typo fix while at it.

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 1/2] io: fix setting of QIO_CHANNEL_FEATURE_FD_PASS on server connections

[Daniel P. Berrange]

On Tue, Dec 22, 2015 at 11:14:41AM -0700, Eric Blake wrote:
> On 12/21/2015 09:23 AM, Daniel P. Berrange wrote:
> > 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.
> > 
> > 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) {
> 
> Looks like a typo fix while at it.

Yes, the latter fix exposed the need for the former fix. I'll mention this
in the commit message too

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

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

[Qemu-devel] [PATCH 2/2] 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.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-socket.c            |  7 ++-
 tests/test-io-channel-socket.c | 98 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 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 44b5de7..74e9046 100644
--- a/tests/test-io-channel-socket.c
+++ b/tests/test-io-channel-socket.c
@@ -376,6 +376,102 @@ 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);
+    if (fdrecv != NULL) {
+        for (i = 0; i < nfdrecv; i++) {
+            close(fdrecv[i]);
+        }
+        g_free(fdrecv);
+    }
+}
 #endif /* _WIN32 */
 
 
@@ -414,6 +510,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] [PATCH 2/2] io: fix stack allocation when sending of file descriptors

[Eric Blake]

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

On 12/21/2015 09:23 AM, Daniel P. Berrange wrote:
> 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.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  io/channel-socket.c            |  7 ++-
>  tests/test-io-channel-socket.c | 98 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 101 insertions(+), 4 deletions(-)
> 

The fix itself is obvious from the commit message; the bulk of this
patch is the testsuite addition (which is a GOOD thing - thanks!).

> +    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);
> +    }

Here, you blindly dereference fdrecv[]...

> +    unlink(TEST_FILE);
> +    close(testfd);
> +    if (fdrecv != NULL) {

...so this if() is dead, and you can just always do the cleanup.

That's minor, so:
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 2/2] io: fix stack allocation when sending of file descriptors

[Daniel P. Berrange]

On Tue, Dec 22, 2015 at 11:20:30AM -0700, Eric Blake wrote:
> On 12/21/2015 09:23 AM, Daniel P. Berrange wrote:
> > 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.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  io/channel-socket.c            |  7 ++-
> >  tests/test-io-channel-socket.c | 98 ++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 101 insertions(+), 4 deletions(-)
> > 
> 
> The fix itself is obvious from the commit message; the bulk of this
> patch is the testsuite addition (which is a GOOD thing - thanks!).

Yes, I wasted lots of time trying to find the flaw before
I wrote the test case at which point it was trivial to
find with valgrind :-)

> 
> > +    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);
> > +    }
> 
> Here, you blindly dereference fdrecv[]...
> 
> > +    unlink(TEST_FILE);
> > +    close(testfd);
> > +    if (fdrecv != NULL) {
> 
> ...so this if() is dead, and you can just always do the cleanup.

Yep, will fix

> That's minor, so:
> Reviewed-by: Eric Blake <eblake@redhat.com>

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