Re: [Qemu-devel] [PATCH] tests/vhost-user-test: Fix potential use-after-free

[Marc-André Lureau]

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
> 
>