From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34110) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTAUd-00070V-P7 for qemu-devel@nongnu.org; Thu, 27 Mar 2014 09:37:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WTAUY-0002uL-IF for qemu-devel@nongnu.org; Thu, 27 Mar 2014 09:37:15 -0400 Received: from mail-oa0-x231.google.com ([2607:f8b0:4003:c02::231]:57220) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTAUY-0002uH-Bt for qemu-devel@nongnu.org; Thu, 27 Mar 2014 09:37:10 -0400 Received: by mail-oa0-f49.google.com with SMTP id h16so4294051oag.8 for ; Thu, 27 Mar 2014 06:37:09 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1395927271.19041.10.camel@localhost.localdomain> References: <1394703694-3281-1-git-send-email-stefanha@redhat.com> <1394703694-3281-3-git-send-email-stefanha@redhat.com> <87vbuzokj8.fsf@blackfin.pond.sub.org> <533423D2.6030402@suse.de> <1395927271.19041.10.camel@localhost.localdomain> Date: Thu, 27 Mar 2014 14:37:09 +0100 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/2] qtest: fix crash if SIGABRT during qtest_init() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: qemu-devel , Stefan Hajnoczi , =?ISO-8859-1?Q?Andreas_F=E4rber?= , Anthony Liguori , Markus Armbruster On Thu, Mar 27, 2014 at 2:34 PM, Marcel Apfelbaum wro= te: > On Thu, 2014-03-27 at 14:12 +0100, Andreas F=E4rber wrote: >> Am 27.03.2014 14:09, schrieb Markus Armbruster: >> > Reply after commit, sorry. >> > >> > Stefan Hajnoczi writes: >> > >> >> If an assertion fails during qtest_init() the SIGABRT handler is >> >> invoked. This is the correct behavior since we need to kill the QEMU >> >> process to avoid leaking it when the test dies. >> >> >> >> The global_qtest pointer used by the SIGABRT handler is currently onl= y >> >> assigned after qtest_init() returns. This results in a segfault if a= n >> >> assertion failure occurs during qtest_init(). >> >> >> >> Move global_qtest assignment inside qtest_init(). Not pretty but let= 's >> >> face it - the signal handler dependeds on global state. >> >> >> >> Reported-by: Marcel Apfelbaum >> >> Signed-off-by: Stefan Hajnoczi >> >> --- >> >> tests/libqtest.c | 3 ++- >> >> tests/libqtest.h | 4 +--- >> >> 2 files changed, 3 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/tests/libqtest.c b/tests/libqtest.c >> >> index c9e78aa..f387662 100644 >> >> --- a/tests/libqtest.c >> >> +++ b/tests/libqtest.c >> >> @@ -120,7 +120,7 @@ QTestState *qtest_init(const char *extra_args) >> >> qemu_binary =3D getenv("QTEST_QEMU_BINARY"); >> >> g_assert(qemu_binary !=3D NULL); >> >> >> >> - s =3D g_malloc(sizeof(*s)); >> >> + global_qtest =3D s =3D g_malloc(sizeof(*s)); >> >> >> >> socket_path =3D g_strdup_printf("/tmp/qtest-%d.sock", getpid()); >> >> qmp_socket_path =3D g_strdup_printf("/tmp/qtest-%d.qmp", getpid(= )); >> >> @@ -181,6 +181,7 @@ QTestState *qtest_init(const char *extra_args) >> >> void qtest_quit(QTestState *s) >> >> { >> >> sigaction(SIGABRT, &s->sigact_old, NULL); >> >> + global_qtest =3D NULL; >> >> >> >> kill_qemu(s); >> >> close(s->fd); >> >> diff --git a/tests/libqtest.h b/tests/libqtest.h >> >> index 9deebdc..7e23a4e 100644 >> >> --- a/tests/libqtest.h >> >> +++ b/tests/libqtest.h >> >> @@ -335,8 +335,7 @@ void qtest_add_func(const char *str, void (*fn)); >> >> */ >> >> static inline QTestState *qtest_start(const char *args) >> >> { >> >> - global_qtest =3D qtest_init(args); >> >> - return global_qtest; >> >> + return qtest_init(args); >> >> } >> >> >> >> /** >> >> @@ -347,7 +346,6 @@ static inline QTestState *qtest_start(const char = *args) >> >> static inline void qtest_end(void) >> >> { >> >> qtest_quit(global_qtest); >> >> - global_qtest =3D NULL; >> >> } >> >> >> >> /** >> > >> > Before this patch, the libqtest API could theoretically support multip= le >> > simultaneous instances of QTestState. This patch kills that option, >> > doesn't it? >> >> Ouch, I thought I had looked out for that... >> >> > >> > If yes: fine with me, we don't need it anyway. >> >> We do. Migration and ivshmem are examples that need two machines - might >> explain why my ivshmem-test was behaving unexpectedly. >> >> Apart from reverting, what are our options? > The problem is in 'kill_qemu' function, which is called from > SIGABRT signal handler. The later needs to know the QTestState > in order to kill the QEMU process. > > Without this patch, kill_qemu will cause a segfault because of: > static void kill_qemu(QTestState *s) > { > if (s->qemu_pid !=3D -1) { > ... > s can be NULL if there is an assert in qtest_init. > > I suppose we can find a different approach, like keeping > the qemu_pid(s) in another statically defined struct. We can keep a global linked list of QEMU pids. Stefan