* [Qemu-devel] [PATCH] tests/vhost-user-test: Fix potential use-after-free
@ 2015-12-02 2:46 David Gibson
2015-12-02 10:36 ` Marc-André Lureau
0 siblings, 1 reply; 3+ messages in thread
From: David Gibson @ 2015-12-02 2:46 UTC (permalink / raw)
To: mst, marcandre.lureau; +Cc: qemu-devel, David Gibson
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 <david@gibson.dropbear.id.au>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/vhost-user-test: Fix potential use-after-free
2015-12-02 2:46 [Qemu-devel] [PATCH] tests/vhost-user-test: Fix potential use-after-free David Gibson
@ 2015-12-02 10:36 ` Marc-André Lureau
2015-12-04 9:05 ` David Gibson
0 siblings, 1 reply; 3+ messages in thread
From: Marc-André Lureau @ 2015-12-02 10:36 UTC (permalink / raw)
To: David Gibson; +Cc: marcandre lureau, qemu-devel, mst
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 <david@gibson.dropbear.id.au>
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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/vhost-user-test: Fix potential use-after-free
2015-12-02 10:36 ` Marc-André Lureau
@ 2015-12-04 9:05 ` David Gibson
0 siblings, 0 replies; 3+ messages in thread
From: David Gibson @ 2015-12-04 9:05 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: marcandre lureau, qemu-devel, mst
[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]
On Wed, Dec 02, 2015 at 05:36:49AM -0500, Marc-André Lureau wrote:
> 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 <david@gibson.dropbear.id.au>
>
> 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"
Drat, wish I'd spotted it. Oh well.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-12-04 9:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-02 2:46 [Qemu-devel] [PATCH] tests/vhost-user-test: Fix potential use-after-free David Gibson
2015-12-02 10:36 ` Marc-André Lureau
2015-12-04 9:05 ` David Gibson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.