diff mbox

[2/2] io: fix stack allocation when sending of file descriptors

Message ID 1450715016-18230-3-git-send-email-berrange@redhat.com
State New
Headers show

Commit Message

Daniel P. Berrangé Dec. 21, 2015, 4:23 p.m. UTC 
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(-)

Comments

Eric Blake Dec. 22, 2015, 6:20 p.m. UTC | #1 
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>
Daniel P. Berrangé Dec. 23, 2015, 10:50 a.m. UTC | #2 
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
diff mbox

Patch

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