Commit 68066019 authored by Marc-André Lureau's avatar Marc-André Lureau
Browse files

char: fix use-after-free with dup chardev & reconnect 



With a reconnect socket, qemu_char_open() will start a background
thread. It should keep a reference on the chardev.

Fixes invalid read:
READ of size 8 at 0x6040000ac858 thread T7
    #0 0x5555598d37b8 in unix_connect_saddr /home/elmarco/src/qq/util/qemu-sockets.c:954
    #1 0x5555598d4751 in socket_connect /home/elmarco/src/qq/util/qemu-sockets.c:1109
    #2 0x555559707c34 in qio_channel_socket_connect_sync /home/elmarco/src/qq/io/channel-socket.c:145
    #3 0x5555596adebb in tcp_chr_connect_client_task /home/elmarco/src/qq/chardev/char-socket.c:1104
    #4 0x555559723d55 in qio_task_thread_worker /home/elmarco/src/qq/io/task.c:123
    #5 0x5555598a6731 in qemu_thread_start /home/elmarco/src/qq/util/qemu-thread-posix.c:519
    #6 0x7ffff40d4431 in start_thread (/lib64/libpthread.so.0+0x9431)
    #7 0x7ffff40029d2 in __clone (/lib64/libc.so.6+0x1019d2)

Signed-off-by: default avatarMarc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: default avatarDaniel P. Berrangé <berrange@redhat.com>
Message-Id: <20200420112012.567284-1-marcandre.lureau@redhat.com>
parent 14a7a203

chardev/char-socket.c

+2 −1
Original line number Diff line number Diff line
@@ -1129,7 +1129,8 @@ static void tcp_chr_connect_client_async(Chardev *chr) 
     */

    s->connect_task = qio_task_new(OBJECT(sioc),

                                   qemu_chr_socket_connected,

                                   chr, NULL);

                                   object_ref(OBJECT(chr)),

                                   (GDestroyNotify)object_unref);

    qio_task_run_in_thread(s->connect_task,

                           tcp_chr_connect_client_task,

                           s->addr,

tests/test-char.c

+52 −2
Original line number Diff line number Diff line
@@ -904,6 +904,52 @@ typedef struct { 
    char_socket_cb event_cb;

} CharSocketClientTestConfig;



static void char_socket_client_dupid_test(gconstpointer opaque)

{

    const CharSocketClientTestConfig *config = opaque;

    QIOChannelSocket *ioc;

    char *optstr;

    Chardev *chr1, *chr2;

    SocketAddress *addr;

    QemuOpts *opts;

    Error *local_err = NULL;



    /*

     * Setup a listener socket and determine get its address

     * so we know the TCP port for the client later

     */

    ioc = qio_channel_socket_new();

    g_assert_nonnull(ioc);

    qio_channel_socket_listen_sync(ioc, config->addr, 1, &error_abort);

    addr = qio_channel_socket_get_local_address(ioc, &error_abort);

    g_assert_nonnull(addr);



    /*

     * Populate the chardev address based on what the server

     * is actually listening on

     */

    optstr = char_socket_addr_to_opt_str(addr,

                                         config->fd_pass,

                                         config->reconnect,

                                         false);



    opts = qemu_opts_parse_noisily(qemu_find_opts("chardev"),

                                   optstr, true);

    g_assert_nonnull(opts);

    chr1 = qemu_chr_new_from_opts(opts, NULL, &error_abort);

    g_assert_nonnull(chr1);



    chr2 = qemu_chr_new_from_opts(opts, NULL, &local_err);

    g_assert_null(chr2);

    error_free_or_abort(&local_err);



    object_unref(OBJECT(ioc));

    qemu_opts_del(opts);

    object_unparent(OBJECT(chr1));

    qapi_free_SocketAddress(addr);

    g_free(optstr);

}



static void char_socket_client_test(gconstpointer opaque)

{

    const CharSocketClientTestConfig *config = opaque;
@@ -1470,6 +1516,8 @@ int main(int argc, char **argv) 
    static CharSocketClientTestConfig client7 ## name =                 \

        { addr, ",reconnect=1", true, false,                            \

            char_socket_event_with_error };                             \

    static CharSocketClientTestConfig client8 ## name =                 \

        { addr, ",reconnect=1", false, false, char_socket_event };      \

    g_test_add_data_func("/char/socket/client/mainloop/" # name,        \

                         &client1 ##name, char_socket_client_test);     \

    g_test_add_data_func("/char/socket/client/wait-conn/" # name,       \
@@ -1483,7 +1531,9 @@ int main(int argc, char **argv) 
    g_test_add_data_func("/char/socket/client/wait-conn-fdpass/" # name, \

                         &client6 ##name, char_socket_client_test);     \

    g_test_add_data_func("/char/socket/client/reconnect-error/" # name, \

                         &client7 ##name, char_socket_client_test)

                         &client7 ##name, char_socket_client_test);     \

    g_test_add_data_func("/char/socket/client/dupid-reconnect/" # name, \

                         &client8 ##name, char_socket_client_dupid_test)



    if (has_ipv4) {

        SOCKET_SERVER_TEST(tcp, &tcpaddr);