[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] tests/vhost-user-test: Fix potential use-after-
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH] tests/vhost-user-test: Fix potential use-after-free |
Date: |
Wed, 2 Dec 2015 05:36:49 -0500 (EST) |
Hi
----- Original Message -----
> ae31fb5 "vhost-user-test: wrap server in TestServer struct" cleaned up
> the handling of the test server in vhost-user-test. Unfortunately it
> introduced a subtle use-after-free if a race goes the wrong way.
>
> When the server structure is freed inside test_server_free() the GThread
> started earlier is still running inside g_main_loop_run(). That GMainLoop
> still has handlers active which reference the server structure, so if those
> trip before the program exits there's a use-after-free.
>
> I've had difficulty reproducing this locally, but for some reason it seems
> to trip every time on Travis builds - this has been breaking all my test
> builds there, which is why I notced it.
>
> This patch prevents the use after free. Unfortunately it looks like there
> are additional problems still breaking my Travis builds, but one problem
> at a time.
>
> Signed-off-by: David Gibson <address@hidden>
The fix is on the ML for a few days, see "vhost-user-test: fix chardriver race"
The last series of fixes is "[PATCH for-2.5 v4 0/4] vhost-user-test fixes"
> ---
> tests/vhost-user-test.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> NOTE: I'm not sure if fixing the race like this is the right approach.
> It might be simpler just to remove the test_server_free() entirely,
> letting the structure leak, since the program is about to terminate
> anyway.
>
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index e4c36af..c547165 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -216,9 +216,10 @@ static void read_guest_mem(TestServer *s)
>
> static void *thread_function(void *data)
> {
> - GMainLoop *loop;
> - loop = g_main_loop_new(NULL, FALSE);
> - g_main_loop_run(loop);
> + GMainLoop **loopp = data;
> +
> + *loopp = g_main_loop_new(NULL, FALSE);
> + g_main_loop_run(*loopp);
> return NULL;
> }
>
> @@ -590,6 +591,8 @@ int main(int argc, char **argv)
> char *qemu_cmd = NULL;
> int ret;
> char template[] = "/tmp/vhost-test-XXXXXX";
> + GThread *thread;
> + GMainLoop *loop;
>
> g_test_init(&argc, &argv, NULL);
>
> @@ -613,7 +616,7 @@ int main(int argc, char **argv)
> server = test_server_new("test");
>
> /* run the main loop thread so the chardev may operate */
> - g_thread_new(NULL, thread_function, NULL);
> + thread = g_thread_new(NULL, thread_function, &loop);
>
> qemu_cmd = GET_QEMU_CMD(server);
>
> @@ -629,6 +632,9 @@ int main(int argc, char **argv)
> qtest_quit(s);
> }
>
> + g_main_loop_quit(loop);
> + g_thread_join(thread);
> +
> /* cleanup */
> test_server_free(server);
>
> --
> 2.5.0
>
>