| Message ID | 1449024397-9200-1-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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 > >
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.
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);
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.