Commit 30bdb3c5 authored by Daniel P. Berrangé's avatar Daniel P. Berrangé
Browse files

sockets: check that the named file descriptor is a socket 



The SocketAddress struct has an "fd" type, which references the name of a
file descriptor passed over the monitor using the "getfd" command. We
currently blindly assume the FD is a socket, which can lead to hard to
diagnose errors later. This adds an explicit check that the FD is actually
a socket to improve the error diagnosis.

Reviewed-by: default avatarEric Blake <eblake@redhat.com>
Reviewed-by: default avatarMarc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: default avatarDaniel P. Berrange <berrange@redhat.com>
parent 58dc31f1

tests/test-util-sockets.c

+89 −0
Original line number Diff line number Diff line
@@ -23,6 +23,7 @@ 
#include "qemu/sockets.h"

#include "qapi/error.h"

#include "socket-helpers.h"

#include "monitor/monitor.h"



static void test_fd_is_socket_bad(void)

{
@@ -49,6 +50,90 @@ static void test_fd_is_socket_good(void) 
    close(fd);

}



static int mon_fd = -1;

static const char *mon_fdname;



int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)

{

    g_assert(cur_mon);

    g_assert(mon == cur_mon);

    if (mon_fd == -1 || !g_str_equal(mon_fdname, fdname)) {

        error_setg(errp, "No fd named %s", fdname);

        return -1;

    }

    return dup(mon_fd);

}



/* Syms in libqemustub.a are discarded at .o file granularity.

 * To replace monitor_get_fd() we must ensure everything in

 * stubs/monitor.c is defined, to make sure monitor.o is discarded

 * otherwise we get duplicate syms at link time.

 */

Monitor *cur_mon;

void monitor_init(Chardev *chr, int flags) {}





static void test_socket_fd_pass_good(void)

{

    SocketAddress addr;

    int fd;



    cur_mon = g_malloc(1); /* Fake a monitor */

    mon_fdname = "myfd";

    mon_fd = qemu_socket(AF_INET, SOCK_STREAM, 0);

    g_assert_cmpint(mon_fd, >, STDERR_FILENO);



    addr.type = SOCKET_ADDRESS_TYPE_FD;

    addr.u.fd.str = g_strdup(mon_fdname);



    fd = socket_connect(&addr, &error_abort);

    g_assert_cmpint(fd, !=, -1);

    g_assert_cmpint(fd, !=, mon_fd);

    close(fd);



    fd = socket_listen(&addr, &error_abort);

    g_assert_cmpint(fd, !=, -1);

    g_assert_cmpint(fd, !=, mon_fd);

    close(fd);



    g_free(addr.u.fd.str);

    mon_fdname = NULL;

    close(mon_fd);

    mon_fd = -1;

    g_free(cur_mon);

    cur_mon = NULL;

}



static void test_socket_fd_pass_bad(void)

{

    SocketAddress addr;

    Error *err = NULL;

    int fd;



    cur_mon = g_malloc(1); /* Fake a monitor */

    mon_fdname = "myfd";

    mon_fd = dup(STDOUT_FILENO);

    g_assert_cmpint(mon_fd, >, STDERR_FILENO);



    addr.type = SOCKET_ADDRESS_TYPE_FD;

    addr.u.fd.str = g_strdup(mon_fdname);



    fd = socket_connect(&addr, &err);

    g_assert_cmpint(fd, ==, -1);

    error_free_or_abort(&err);



    fd = socket_listen(&addr, &err);

    g_assert_cmpint(fd, ==, -1);

    error_free_or_abort(&err);



    g_free(addr.u.fd.str);

    mon_fdname = NULL;

    close(mon_fd);

    mon_fd = -1;

    g_free(cur_mon);

    cur_mon = NULL;

}



int main(int argc, char **argv)

{

    bool has_ipv4, has_ipv6;
@@ -71,6 +156,10 @@ int main(int argc, char **argv) 
                        test_fd_is_socket_bad);

        g_test_add_func("/util/socket/is-socket/good",

                        test_fd_is_socket_good);

        g_test_add_func("/socket/fd-pass/good",

                        test_socket_fd_pass_good);

        g_test_add_func("/socket/fd-pass/bad",

                        test_socket_fd_pass_bad);

    }



    return g_test_run();

util/qemu-sockets.c

+16 −2
Original line number Diff line number Diff line
@@ -1042,6 +1042,20 @@ fail: 
    return NULL;

}



static int socket_get_fd(const char *fdstr, Error **errp)

{

    int fd = monitor_get_fd(cur_mon, fdstr, errp);

    if (fd < 0) {

        return -1;

    }

    if (!fd_is_socket(fd)) {

        error_setg(errp, "File descriptor '%s' is not a socket", fdstr);

        close(fd);

        return -1;

    }

    return fd;

}



int socket_connect(SocketAddress *addr, Error **errp)

{

    int fd;
@@ -1056,7 +1070,7 @@ int socket_connect(SocketAddress *addr, Error **errp) 
        break;



    case SOCKET_ADDRESS_TYPE_FD:

        fd = monitor_get_fd(cur_mon, addr->u.fd.str, errp);

        fd = socket_get_fd(addr->u.fd.str, errp);

        break;



    case SOCKET_ADDRESS_TYPE_VSOCK:
@@ -1083,7 +1097,7 @@ int socket_listen(SocketAddress *addr, Error **errp) 
        break;



    case SOCKET_ADDRESS_TYPE_FD:

        fd = monitor_get_fd(cur_mon, addr->u.fd.str, errp);

        fd = socket_get_fd(addr->u.fd.str, errp);

        break;



    case SOCKET_ADDRESS_TYPE_VSOCK: