[Qemu-devel] [PATCH 0/3] qtest: avoid pidfile and QEMU process leaks

[Qemu-devel] [PATCH 0/3] qtest: avoid pidfile and QEMU process leaks

[Stefan Hajnoczi]

This series prevents the following qtest issues:

1. Leaking the pidfile if QEMU startup fails, as discovered by Andreas Färber.
2. Leaking the QEMU process when a test case aborts.

Applying this series should make buildbots and manual "make check" users have a
more pleasant and less leaky experience :).

Stefan Hajnoczi (3):
  qtest: drop unused child_pid field
  qtest: make QEMU our direct child process
  qtest: kill QEMU process on g_assert() failure

 tests/libqtest.c | 59 +++++++++++++++++++++++++++-----------------------------
 1 file changed, 28 insertions(+), 31 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH 1/3] qtest: drop unused child_pid field

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqtest.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index c9a4f89..2876ce4 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -43,7 +43,6 @@ struct QTestState
     int qmp_fd;
     bool irq_level[MAX_IRQ];
     GString *rx;
-    int child_pid;   /* Child process created to execute QEMU */
     pid_t qemu_pid;  /* QEMU process spawned by our child */
 };
 
@@ -152,7 +151,6 @@ QTestState *qtest_init(const char *extra_args)
     g_free(qmp_socket_path);
 
     s->rx = g_string_new("");
-    s->child_pid = pid;
     for (i = 0; i < MAX_IRQ; i++) {
         s->irq_level[i] = false;
     }
-- 
1.8.5.3

[Qemu-devel] [PATCH 2/3] qtest: make QEMU our direct child process

[Stefan Hajnoczi]

qtest_init() cannot use exec*p() to launch QEMU since the exec*p()
functions take an argument array while qtest_init() takes char
*extra_args.  Therefore we execute /bin/sh -c <command-line> and let the
shell parse the argument string.

This left /bin/sh as our child process and our child's child was QEMU.
We still want QEMU's pid so the -pidfile option was used to let QEMU
report its pid.

The pidfile needs to be unlinked when the test case exits or fails.  In
other words, the pidfile creates a new problem for us!

Simplify all this using the shell 'exec' command.  It allows us to
replace the /bin/sh process with QEMU.  Then we no longer need to use
-pidfile because we already know our fork child's pid.

Note: Yes, it seems silly to exec /bin/sh when we could just exec QEMU
directly.  But remember qtest_init() takes a single char *extra_args
command-line fragment instead of a real argv[] array, so we need
/bin/sh's argument parsing behavior.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqtest.c | 34 +++++-----------------------------
 1 file changed, 5 insertions(+), 29 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 2876ce4..8b2b2d7 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -43,7 +43,7 @@ struct QTestState
     int qmp_fd;
     bool irq_level[MAX_IRQ];
     GString *rx;
-    pid_t qemu_pid;  /* QEMU process spawned by our child */
+    pid_t qemu_pid;  /* our child QEMU process */
 };
 
 #define g_assert_no_errno(ret) do { \
@@ -88,32 +88,14 @@ static int socket_accept(int sock)
     return ret;
 }
 
-static pid_t read_pid_file(const char *pid_file)
-{
-    FILE *f;
-    char buffer[1024];
-    pid_t pid = -1;
-
-    f = fopen(pid_file, "r");
-    if (f) {
-        if (fgets(buffer, sizeof(buffer), f)) {
-            pid = atoi(buffer);
-        }
-        fclose(f);
-    }
-    return pid;
-}
-
 QTestState *qtest_init(const char *extra_args)
 {
     QTestState *s;
     int sock, qmpsock, i;
     gchar *socket_path;
     gchar *qmp_socket_path;
-    gchar *pid_file;
     gchar *command;
     const char *qemu_binary;
-    pid_t pid;
 
     qemu_binary = getenv("QTEST_QEMU_BINARY");
     g_assert(qemu_binary != NULL);
@@ -122,22 +104,20 @@ QTestState *qtest_init(const char *extra_args)
 
     socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
     qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
-    pid_file = g_strdup_printf("/tmp/qtest-%d.pid", getpid());
 
     sock = init_socket(socket_path);
     qmpsock = init_socket(qmp_socket_path);
 
-    pid = fork();
-    if (pid == 0) {
-        command = g_strdup_printf("%s "
+    s->qemu_pid = fork();
+    if (s->qemu_pid == 0) {
+        command = g_strdup_printf("exec %s "
                                   "-qtest unix:%s,nowait "
                                   "-qtest-log /dev/null "
                                   "-qmp unix:%s,nowait "
-                                  "-pidfile %s "
                                   "-machine accel=qtest "
                                   "-display none "
                                   "%s", qemu_binary, socket_path,
-                                  qmp_socket_path, pid_file,
+                                  qmp_socket_path,
                                   extra_args ?: "");
         execlp("/bin/sh", "sh", "-c", command, NULL);
         exit(1);
@@ -159,10 +139,6 @@ QTestState *qtest_init(const char *extra_args)
     qtest_qmp_discard_response(s, "");
     qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
 
-    s->qemu_pid = read_pid_file(pid_file);
-    unlink(pid_file);
-    g_free(pid_file);
-
     if (getenv("QTEST_STOP")) {
         kill(s->qemu_pid, SIGSTOP);
     }
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH 2/3] qtest: make QEMU our direct child process

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> qtest_init() cannot use exec*p() to launch QEMU since the exec*p()
> functions take an argument array while qtest_init() takes char
> *extra_args.  Therefore we execute /bin/sh -c <command-line> and let the
> shell parse the argument string.
>
> This left /bin/sh as our child process and our child's child was QEMU.
> We still want QEMU's pid so the -pidfile option was used to let QEMU
> report its pid.
>
> The pidfile needs to be unlinked when the test case exits or fails.  In
> other words, the pidfile creates a new problem for us!
>
> Simplify all this using the shell 'exec' command.  It allows us to
> replace the /bin/sh process with QEMU.  Then we no longer need to use
> -pidfile because we already know our fork child's pid.
>
> Note: Yes, it seems silly to exec /bin/sh when we could just exec QEMU
> directly.  But remember qtest_init() takes a single char *extra_args
> command-line fragment instead of a real argv[] array, so we need
> /bin/sh's argument parsing behavior.

Sounds like a design mistake to me.

> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Patch looks good.

Re: [Qemu-devel] [PATCH 2/3] qtest: make QEMU our direct child process

[Stefan Hajnoczi]

On Mon, Feb 17, 2014 at 05:44:55PM +0100, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > qtest_init() cannot use exec*p() to launch QEMU since the exec*p()
> > functions take an argument array while qtest_init() takes char
> > *extra_args.  Therefore we execute /bin/sh -c <command-line> and let the
> > shell parse the argument string.
> >
> > This left /bin/sh as our child process and our child's child was QEMU.
> > We still want QEMU's pid so the -pidfile option was used to let QEMU
> > report its pid.
> >
> > The pidfile needs to be unlinked when the test case exits or fails.  In
> > other words, the pidfile creates a new problem for us!
> >
> > Simplify all this using the shell 'exec' command.  It allows us to
> > replace the /bin/sh process with QEMU.  Then we no longer need to use
> > -pidfile because we already know our fork child's pid.
> >
> > Note: Yes, it seems silly to exec /bin/sh when we could just exec QEMU
> > directly.  But remember qtest_init() takes a single char *extra_args
> > command-line fragment instead of a real argv[] array, so we need
> > /bin/sh's argument parsing behavior.
> 
> Sounds like a design mistake to me.

I wouldn't call char *extra_args a mistake because strings are still
simpler to manipulate in C than char *argv[] arrays.  So we write less
code in test cases and libqtest.c at the expense of a roundabout way to
spawn the process.

Stefan

Re: [Qemu-devel] [PATCH 2/3] qtest: make QEMU our direct child process

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Mon, Feb 17, 2014 at 05:44:55PM +0100, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>> 
>> > qtest_init() cannot use exec*p() to launch QEMU since the exec*p()
>> > functions take an argument array while qtest_init() takes char
>> > *extra_args.  Therefore we execute /bin/sh -c <command-line> and let the
>> > shell parse the argument string.
>> >
>> > This left /bin/sh as our child process and our child's child was QEMU.
>> > We still want QEMU's pid so the -pidfile option was used to let QEMU
>> > report its pid.
>> >
>> > The pidfile needs to be unlinked when the test case exits or fails.  In
>> > other words, the pidfile creates a new problem for us!
>> >
>> > Simplify all this using the shell 'exec' command.  It allows us to
>> > replace the /bin/sh process with QEMU.  Then we no longer need to use
>> > -pidfile because we already know our fork child's pid.
>> >
>> > Note: Yes, it seems silly to exec /bin/sh when we could just exec QEMU
>> > directly.  But remember qtest_init() takes a single char *extra_args
>> > command-line fragment instead of a real argv[] array, so we need
>> > /bin/sh's argument parsing behavior.
>> 
>> Sounds like a design mistake to me.
>
> I wouldn't call char *extra_args a mistake because strings are still
> simpler to manipulate in C than char *argv[] arrays.  So we write less
> code in test cases and libqtest.c at the expense of a roundabout way to
> spawn the process.

Building command arguments in a string is deceptively simple.  The
deception collapses once you add funny characters that make the shell do
funny and unexpected things.

Our extra_args string manipulations are typically of the form "format a
bunch of options and option arguments into a string".  I suspect using a
function to collect a bunch of options and option arguments into an
array would be about the same amount of code in most cases.

Just sayin'; I'm not objecting to your patch.

[Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Stefan Hajnoczi]

The QEMU process stays running if the test case fails.  This patch fixes
the leak by installing a SIGABRT signal handler which invokes
qtest_end().

In order to make that work for assertion failures during qtest_init(),
we need to initialize QTestState fields including file descriptors and
pids carefully.  qtest_quit() is then safe to call even during
qtest_init().

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/libqtest.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 8b2b2d7..09a0481 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -44,6 +44,7 @@ struct QTestState
     bool irq_level[MAX_IRQ];
     GString *rx;
     pid_t qemu_pid;  /* our child QEMU process */
+    struct sigaction sigact_old; /* restored on exit */
 };
 
 #define g_assert_no_errno(ret) do { \
@@ -88,6 +89,11 @@ static int socket_accept(int sock)
     return ret;
 }
 
+static void sigabrt_handler(int signo)
+{
+    qtest_end();
+}
+
 QTestState *qtest_init(const char *extra_args)
 {
     QTestState *s;
@@ -96,11 +102,22 @@ QTestState *qtest_init(const char *extra_args)
     gchar *qmp_socket_path;
     gchar *command;
     const char *qemu_binary;
+    struct sigaction sigact;
 
     qemu_binary = getenv("QTEST_QEMU_BINARY");
     g_assert(qemu_binary != NULL);
 
-    s = g_malloc(sizeof(*s));
+    s = g_malloc0(sizeof(*s));
+    s->fd = -1;
+    s->qmp_fd = -1;
+    s->qemu_pid = -1;
+
+    /* Catch SIGABRT to clean up on g_assert() failure */
+    sigact = (struct sigaction){
+        .sa_handler = sigabrt_handler,
+        .sa_flags = SA_RESETHAND,
+    };
+    sigaction(SIGABRT, &sigact, &s->sigact_old);
 
     socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
     qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
@@ -150,13 +167,19 @@ void qtest_quit(QTestState *s)
 {
     int status;
 
+    sigaction(SIGABRT, &s->sigact_old, NULL);
+
     if (s->qemu_pid != -1) {
         kill(s->qemu_pid, SIGTERM);
         waitpid(s->qemu_pid, &status, 0);
     }
 
-    close(s->fd);
-    close(s->qmp_fd);
+    if (s->fd != -1) {
+        close(s->fd);
+    }
+    if (s->qmp_fd != -1) {
+        close(s->qmp_fd);
+    }
     g_string_free(s->rx, true);
     g_free(s);
 }
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Paolo Bonzini]

Il 17/02/2014 16:44, Stefan Hajnoczi ha scritto:
>  }
>
> +static void sigabrt_handler(int signo)
> +{
> +    qtest_end();
> +}
> +

void qtest_quit(QTestState *s)
{
     int status;

     if (s->qemu_pid != -1) {
         kill(s->qemu_pid, SIGTERM);
         waitpid(s->qemu_pid, &status, 0);
     }

     close(s->fd);
     close(s->qmp_fd);
     g_string_free(s->rx, true);
     g_free(s);
}

Not async-signal safe.  You need to ignore the g_string_free and g_free 
(perhaps even the closes) if calling from the sigabrt_handler.

Paolo

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 17/02/2014 16:44, Stefan Hajnoczi ha scritto:
>>  }
>>
>> +static void sigabrt_handler(int signo)
>> +{
>> +    qtest_end();
>> +}
>> +
>
> void qtest_quit(QTestState *s)
> {
>     int status;
>
>     if (s->qemu_pid != -1) {
>         kill(s->qemu_pid, SIGTERM);
>         waitpid(s->qemu_pid, &status, 0);
>     }
>
>     close(s->fd);
>     close(s->qmp_fd);
>     g_string_free(s->rx, true);
>     g_free(s);
> }
>
> Not async-signal safe.  You need to ignore the g_string_free and
> g_free (perhaps even the closes) if calling from the sigabrt_handler.

kill(), waitpid() and close() are all async-signal-safe.

SIGABRT is normally synchronous enough: it's sent by abort().  But of
course, nothing stops the user from kill -ABRT.  Or GLib from calling
abort() in some place where an attempt to reenter it crashes & burns.
Not sure I'd care, but I'm pretty sure I don't care for freeing stuff on
exit :)

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Stefan Hajnoczi]

On Mon, Feb 17, 2014 at 06:00:03PM +0100, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > Il 17/02/2014 16:44, Stefan Hajnoczi ha scritto:
> >>  }
> >>
> >> +static void sigabrt_handler(int signo)
> >> +{
> >> +    qtest_end();
> >> +}
> >> +
> >
> > void qtest_quit(QTestState *s)
> > {
> >     int status;
> >
> >     if (s->qemu_pid != -1) {
> >         kill(s->qemu_pid, SIGTERM);
> >         waitpid(s->qemu_pid, &status, 0);
> >     }
> >
> >     close(s->fd);
> >     close(s->qmp_fd);
> >     g_string_free(s->rx, true);
> >     g_free(s);
> > }
> >
> > Not async-signal safe.  You need to ignore the g_string_free and
> > g_free (perhaps even the closes) if calling from the sigabrt_handler.
> 
> kill(), waitpid() and close() are all async-signal-safe.
> 
> SIGABRT is normally synchronous enough: it's sent by abort().  But of
> course, nothing stops the user from kill -ABRT.  Or GLib from calling
> abort() in some place where an attempt to reenter it crashes & burns.
> Not sure I'd care, but I'm pretty sure I don't care for freeing stuff on
> exit :)

Yes, SIGABRT is synchronous for all purposes.  So the only danger is
that g_string_free() or g_free() could fail while we're in
g_assert(false).  But they don't, which makes sense because they are
totally unrelated to g_assert() and therefore can handle re-entrancy.

In practice there is no issue and I've tested assertion failure with
glib 1.2.10.

Stefan

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Mon, Feb 17, 2014 at 06:00:03PM +0100, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > Il 17/02/2014 16:44, Stefan Hajnoczi ha scritto:
>> >>  }
>> >>
>> >> +static void sigabrt_handler(int signo)
>> >> +{
>> >> +    qtest_end();
>> >> +}
>> >> +
>> >
>> > void qtest_quit(QTestState *s)
>> > {
>> >     int status;
>> >
>> >     if (s->qemu_pid != -1) {
>> >         kill(s->qemu_pid, SIGTERM);
>> >         waitpid(s->qemu_pid, &status, 0);
>> >     }
>> >
>> >     close(s->fd);
>> >     close(s->qmp_fd);
>> >     g_string_free(s->rx, true);
>> >     g_free(s);
>> > }
>> >
>> > Not async-signal safe.  You need to ignore the g_string_free and
>> > g_free (perhaps even the closes) if calling from the sigabrt_handler.
>> 
>> kill(), waitpid() and close() are all async-signal-safe.
>> 
>> SIGABRT is normally synchronous enough: it's sent by abort().  But of
>> course, nothing stops the user from kill -ABRT.  Or GLib from calling
>> abort() in some place where an attempt to reenter it crashes & burns.
>> Not sure I'd care, but I'm pretty sure I don't care for freeing stuff on
>> exit :)
>
> Yes, SIGABRT is synchronous for all purposes.  So the only danger is
> that g_string_free() or g_free() could fail while we're in
> g_assert(false).  But they don't, which makes sense because they are
> totally unrelated to g_assert() and therefore can handle re-entrancy.

The (theoretical!) problem is when it aborts in the bowels of g_*free(),
and your SIGABRT handler reenters g_*free().

> In practice there is no issue and I've tested assertion failure with
> glib 1.2.10.

Worst that can happen is we crash on the way from abort() to process
termination.  Tolerable.

Still, avoiding unnecessary cleanup on that path seems prudent to me.
If you agree, factor out the kill()/waitpid(), and call only that from
the signal handler.

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Paolo Bonzini]

Il 18/02/2014 11:05, Markus Armbruster ha scritto:
>> > Yes, SIGABRT is synchronous for all purposes.  So the only danger is
>> > that g_string_free() or g_free() could fail while we're in
>> > g_assert(false).  But they don't, which makes sense because they are
>> > totally unrelated to g_assert() and therefore can handle re-entrancy.
> The (theoretical!) problem is when it aborts in the bowels of g_*free(),
> and your SIGABRT handler reenters g_*free().
>
>> > In practice there is no issue and I've tested assertion failure with
>> > glib 1.2.10.
> Worst that can happen is we crash on the way from abort() to process
> termination.  Tolerable.

What about recursive locking of a non-recursive mutex?

Paolo

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 18/02/2014 11:05, Markus Armbruster ha scritto:
>>> > Yes, SIGABRT is synchronous for all purposes.  So the only danger is
>>> > that g_string_free() or g_free() could fail while we're in
>>> > g_assert(false).  But they don't, which makes sense because they are
>>> > totally unrelated to g_assert() and therefore can handle re-entrancy.
>> The (theoretical!) problem is when it aborts in the bowels of g_*free(),
>> and your SIGABRT handler reenters g_*free().
>>
>>> > In practice there is no issue and I've tested assertion failure with
>>> > glib 1.2.10.
>> Worst that can happen is we crash on the way from abort() to process
>> termination.  Tolerable.
>
> What about recursive locking of a non-recursive mutex?

You're right, deadlock is possible.  Strengthens the argument for:

>> Still, avoiding unnecessary cleanup on that path seems prudent to me.
>> If you agree, factor out the kill()/waitpid(), and call only that from
>> the signal handler.

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Stefan Hajnoczi]

On Tue, Feb 18, 2014 at 11:43:10AM +0100, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > Il 18/02/2014 11:05, Markus Armbruster ha scritto:
> >>> > Yes, SIGABRT is synchronous for all purposes.  So the only danger is
> >>> > that g_string_free() or g_free() could fail while we're in
> >>> > g_assert(false).  But they don't, which makes sense because they are
> >>> > totally unrelated to g_assert() and therefore can handle re-entrancy.
> >> The (theoretical!) problem is when it aborts in the bowels of g_*free(),
> >> and your SIGABRT handler reenters g_*free().
> >>
> >>> > In practice there is no issue and I've tested assertion failure with
> >>> > glib 1.2.10.
> >> Worst that can happen is we crash on the way from abort() to process
> >> termination.  Tolerable.
> >
> > What about recursive locking of a non-recursive mutex?
> 
> You're right, deadlock is possible.  Strengthens the argument for:
> 
> >> Still, avoiding unnecessary cleanup on that path seems prudent to me.
> >> If you agree, factor out the kill()/waitpid(), and call only that from
> >> the signal handler.

Okay, I'm convinced.

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Paolo Bonzini]

Il 18/02/2014 10:05, Stefan Hajnoczi ha scritto:
>> > SIGABRT is normally synchronous enough: it's sent by abort().  But of
>> > course, nothing stops the user from kill -ABRT.  Or GLib from calling
>> > abort() in some place where an attempt to reenter it crashes & burns.
>> > Not sure I'd care, but I'm pretty sure I don't care for freeing stuff on
>> > exit :)
> Yes, SIGABRT is synchronous for all purposes.  So the only danger is
> that g_string_free() or g_free() could fail while we're in
> g_assert(false).  But they don't, which makes sense because they are
> totally unrelated to g_assert() and therefore can handle re-entrancy.

If malloc aborts due to a double free or other similar problem, you may 
risk reentering it.

Paolo

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Daniel P. Berrange]

On Tue, Feb 18, 2014 at 11:07:53AM +0100, Paolo Bonzini wrote:
> Il 18/02/2014 10:05, Stefan Hajnoczi ha scritto:
> >>> SIGABRT is normally synchronous enough: it's sent by abort().  But of
> >>> course, nothing stops the user from kill -ABRT.  Or GLib from calling
> >>> abort() in some place where an attempt to reenter it crashes & burns.
> >>> Not sure I'd care, but I'm pretty sure I don't care for freeing stuff on
> >>> exit :)
> >Yes, SIGABRT is synchronous for all purposes.  So the only danger is
> >that g_string_free() or g_free() could fail while we're in
> >g_assert(false).  But they don't, which makes sense because they are
> >totally unrelated to g_assert() and therefore can handle re-entrancy.
> 
> If malloc aborts due to a double free or other similar problem, you
> may risk reentering it.

If you register the custom SIGABRT handler with sigaction + SA_RESETHAND
then you'd avoid the re-entrancy risk, since a cascading SIGABRT would
get handled by the system default handler, which would immediately
terminate the process.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Paolo Bonzini]

Il 18/02/2014 11:17, Daniel P. Berrange ha scritto:
>>> > >Yes, SIGABRT is synchronous for all purposes.  So the only danger is
>>> > >that g_string_free() or g_free() could fail while we're in
>>> > >g_assert(false).  But they don't, which makes sense because they are
>>> > >totally unrelated to g_assert() and therefore can handle re-entrancy.
>> >
>> > If malloc aborts due to a double free or other similar problem, you
>> > may risk reentering it.
> If you register the custom SIGABRT handler with sigaction + SA_RESETHAND
> then you'd avoid the re-entrancy risk, since a cascading SIGABRT would
> get handled by the system default handler, which would immediately
> terminate the process.

I meant reentering malloc.

Paolo

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> The QEMU process stays running if the test case fails.  This patch fixes
> the leak by installing a SIGABRT signal handler which invokes
> qtest_end().
>
> In order to make that work for assertion failures during qtest_init(),
> we need to initialize QTestState fields including file descriptors and
> pids carefully.  qtest_quit() is then safe to call even during
> qtest_init().
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/libqtest.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 8b2b2d7..09a0481 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -44,6 +44,7 @@ struct QTestState
>      bool irq_level[MAX_IRQ];
>      GString *rx;
>      pid_t qemu_pid;  /* our child QEMU process */
> +    struct sigaction sigact_old; /* restored on exit */
>  };
>  
>  #define g_assert_no_errno(ret) do { \
> @@ -88,6 +89,11 @@ static int socket_accept(int sock)
>      return ret;
>  }
>  
> +static void sigabrt_handler(int signo)
> +{
> +    qtest_end();
> +}
> +
>  QTestState *qtest_init(const char *extra_args)
>  {
>      QTestState *s;
> @@ -96,11 +102,22 @@ QTestState *qtest_init(const char *extra_args)
>      gchar *qmp_socket_path;
>      gchar *command;
>      const char *qemu_binary;
> +    struct sigaction sigact;
>  
>      qemu_binary = getenv("QTEST_QEMU_BINARY");
>      g_assert(qemu_binary != NULL);
>  
> -    s = g_malloc(sizeof(*s));
> +    s = g_malloc0(sizeof(*s));
> +    s->fd = -1;
> +    s->qmp_fd = -1;
> +    s->qemu_pid = -1;
> +
> +    /* Catch SIGABRT to clean up on g_assert() failure */
> +    sigact = (struct sigaction){
> +        .sa_handler = sigabrt_handler,
> +        .sa_flags = SA_RESETHAND,
> +    };

Assumes zero-initialization has the same effect as
sigemptyset(&sigact.sa_mask).  Quoting POSIX:

    The implementation of the sigemptyset() (or sigfillset()) function
    could quite trivially clear (or set) all the bits in the signal set.
    Alternatively, it would be reasonable to initialize part of the
    structure, such as a version field, to permit binary-compatibility
    between releases where the size of the set varies.  For such
    reasons, either sigemptyset() or sigfillset() must be called prior
    to any other use of the signal set, even if such use is read-only
    (for example, as an argument to sigpending()).

Looks like you better sigemptyset() here, for maximum portability.

> +    sigaction(SIGABRT, &sigact, &s->sigact_old);
>  
>      socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
>      qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
> @@ -150,13 +167,19 @@ void qtest_quit(QTestState *s)
>  {
>      int status;
>  
> +    sigaction(SIGABRT, &s->sigact_old, NULL);
> +

Can this ever restore the action to anything but the default action?

>      if (s->qemu_pid != -1) {
>          kill(s->qemu_pid, SIGTERM);
>          waitpid(s->qemu_pid, &status, 0);
>      }
>  
> -    close(s->fd);
> -    close(s->qmp_fd);
> +    if (s->fd != -1) {
> +        close(s->fd);
> +    }
> +    if (s->qmp_fd != -1) {
> +        close(s->qmp_fd);
> +    }

I generally don't bother to avoid close(-1).

>      g_string_free(s->rx, true);
>      g_free(s);
>  }

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Paolo Bonzini]

Il 17/02/2014 17:49, Markus Armbruster ha scritto:
> Assumes zero-initialization has the same effect as
> sigemptyset(&sigact.sa_mask).  Quoting POSIX:
>
>     The implementation of the sigemptyset() (or sigfillset()) function
>     could quite trivially clear (or set) all the bits in the signal set.
>     Alternatively, it would be reasonable to initialize part of the
>     structure, such as a version field, to permit binary-compatibility
>     between releases where the size of the set varies.  For such
>     reasons, either sigemptyset() or sigfillset() must be called prior
>     to any other use of the signal set, even if such use is read-only
>     (for example, as an argument to sigpending()).
>
> Looks like you better sigemptyset() here, for maximum portability.
>

Certainly memset of struct sigaction or sigset_t * is common enough that 
no one in their right minds would do this.  Is there really an OS that 
does it?  Also, the above justification is quite feeble; it would work 
for binary compatibility of sigset_t* arguments, but not for embedded 
sigset_t structs.  I'm CCing our resident POSIX experts in hope that 
this paragraph can be eliminated from the standard. :)

Related to this, there are a bunch of Coverity reports where we use 
uninitialized fields of a struct sigaction.

Paolo

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Stefan Hajnoczi]

On Mon, Feb 17, 2014 at 05:49:31PM +0100, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> > +    /* Catch SIGABRT to clean up on g_assert() failure */
> > +    sigact = (struct sigaction){
> > +        .sa_handler = sigabrt_handler,
> > +        .sa_flags = SA_RESETHAND,
> > +    };
> 
> Assumes zero-initialization has the same effect as
> sigemptyset(&sigact.sa_mask).  Quoting POSIX:
> 
>     The implementation of the sigemptyset() (or sigfillset()) function
>     could quite trivially clear (or set) all the bits in the signal set.
>     Alternatively, it would be reasonable to initialize part of the
>     structure, such as a version field, to permit binary-compatibility
>     between releases where the size of the set varies.  For such
>     reasons, either sigemptyset() or sigfillset() must be called prior
>     to any other use of the signal set, even if such use is read-only
>     (for example, as an argument to sigpending()).
> 
> Looks like you better sigemptyset() here, for maximum portability.

Okay, will do that.

> > +    sigaction(SIGABRT, &sigact, &s->sigact_old);
> >  
> >      socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
> >      qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
> > @@ -150,13 +167,19 @@ void qtest_quit(QTestState *s)
> >  {
> >      int status;
> >  
> > +    sigaction(SIGABRT, &s->sigact_old, NULL);
> > +
> 
> Can this ever restore the action to anything but the default action?

This code is in a "library" so I don't want to make assumptions about
the callers.  Keeping sigact_old around is literally 1 line of code so I
think it's worth it.

> >      if (s->qemu_pid != -1) {
> >          kill(s->qemu_pid, SIGTERM);
> >          waitpid(s->qemu_pid, &status, 0);
> >      }
> >  
> > -    close(s->fd);
> > -    close(s->qmp_fd);
> > +    if (s->fd != -1) {
> > +        close(s->fd);
> > +    }
> > +    if (s->qmp_fd != -1) {
> > +        close(s->qmp_fd);
> > +    }
> 
> I generally don't bother to avoid close(-1).

When I drive on the highway I stay on the lanes but I guess I could just
slide along the side barriers :).  It's a style issue but close(-1)
annoys me in strace so I try to avoid doing it.

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Mon, Feb 17, 2014 at 05:49:31PM +0100, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>> > +    /* Catch SIGABRT to clean up on g_assert() failure */
>> > +    sigact = (struct sigaction){
>> > +        .sa_handler = sigabrt_handler,
>> > +        .sa_flags = SA_RESETHAND,
>> > +    };
>> 
>> Assumes zero-initialization has the same effect as
>> sigemptyset(&sigact.sa_mask).  Quoting POSIX:
>> 
>>     The implementation of the sigemptyset() (or sigfillset()) function
>>     could quite trivially clear (or set) all the bits in the signal set.
>>     Alternatively, it would be reasonable to initialize part of the
>>     structure, such as a version field, to permit binary-compatibility
>>     between releases where the size of the set varies.  For such
>>     reasons, either sigemptyset() or sigfillset() must be called prior
>>     to any other use of the signal set, even if such use is read-only
>>     (for example, as an argument to sigpending()).
>> 
>> Looks like you better sigemptyset() here, for maximum portability.
>
> Okay, will do that.
>
>> > +    sigaction(SIGABRT, &sigact, &s->sigact_old);
>> >  
>> >      socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
>> >      qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
>> > @@ -150,13 +167,19 @@ void qtest_quit(QTestState *s)
>> >  {
>> >      int status;
>> >  
>> > +    sigaction(SIGABRT, &s->sigact_old, NULL);
>> > +
>> 
>> Can this ever restore the action to anything but the default action?
>
> This code is in a "library" so I don't want to make assumptions about
> the callers.  Keeping sigact_old around is literally 1 line of code so I
> think it's worth it.
>
>> >      if (s->qemu_pid != -1) {
>> >          kill(s->qemu_pid, SIGTERM);
>> >          waitpid(s->qemu_pid, &status, 0);
>> >      }
>> >  
>> > -    close(s->fd);
>> > -    close(s->qmp_fd);
>> > +    if (s->fd != -1) {
>> > +        close(s->fd);
>> > +    }
>> > +    if (s->qmp_fd != -1) {
>> > +        close(s->qmp_fd);
>> > +    }
>> 
>> I generally don't bother to avoid close(-1).
>
> When I drive on the highway I stay on the lanes but I guess I could just
> slide along the side barriers :).  It's a style issue but close(-1)
> annoys me in strace so I try to avoid doing it.

For me, it's in the same category as free(NULL).

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Stefan Hajnoczi]

On Tue, Feb 18, 2014 at 10:55:56AM +0100, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
> 
> > On Mon, Feb 17, 2014 at 05:49:31PM +0100, Markus Armbruster wrote:
> >> Stefan Hajnoczi <stefanha@redhat.com> writes:
> >> >      if (s->qemu_pid != -1) {
> >> >          kill(s->qemu_pid, SIGTERM);
> >> >          waitpid(s->qemu_pid, &status, 0);
> >> >      }
> >> >  
> >> > -    close(s->fd);
> >> > -    close(s->qmp_fd);
> >> > +    if (s->fd != -1) {
> >> > +        close(s->fd);
> >> > +    }
> >> > +    if (s->qmp_fd != -1) {
> >> > +        close(s->qmp_fd);
> >> > +    }
> >> 
> >> I generally don't bother to avoid close(-1).
> >
> > When I drive on the highway I stay on the lanes but I guess I could just
> > slide along the side barriers :).  It's a style issue but close(-1)
> > annoys me in strace so I try to avoid doing it.
> 
> For me, it's in the same category as free(NULL).

Understood.  I'll drop it from the next patch.

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Peter Maydell]

On 18 February 2014 09:17, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> When I drive on the highway I stay on the lanes but I guess I could just
> slide along the side barriers :).  It's a style issue but close(-1)
> annoys me in strace so I try to avoid doing it.

As an aside, ever try stracing gtester? It implements its "-o file"
option by always doing the write of the XML, so if you didn't
specify -o then the write()s and close() are all done on fd -1...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
>> The QEMU process stays running if the test case fails.  This patch fixes
>> the leak by installing a SIGABRT signal handler which invokes
>> qtest_end().
>>
>> In order to make that work for assertion failures during qtest_init(),
>> we need to initialize QTestState fields including file descriptors and
>> pids carefully.  qtest_quit() is then safe to call even during
>> qtest_init().
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  tests/libqtest.c | 29 ++++++++++++++++++++++++++---
>>  1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index 8b2b2d7..09a0481 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -44,6 +44,7 @@ struct QTestState
>>      bool irq_level[MAX_IRQ];
>>      GString *rx;
>>      pid_t qemu_pid;  /* our child QEMU process */
>> +    struct sigaction sigact_old; /* restored on exit */
>>  };
>>  
>>  #define g_assert_no_errno(ret) do { \
>> @@ -88,6 +89,11 @@ static int socket_accept(int sock)
>>      return ret;
>>  }
>>  
>> +static void sigabrt_handler(int signo)
>> +{
>> +    qtest_end();

Don't you have to re-raise SIGABRT here, to actually terminate the
process?

>> +}
>> +
>>  QTestState *qtest_init(const char *extra_args)
>>  {
>>      QTestState *s;
[...]

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Stefan Hajnoczi]

On Tue, Feb 18, 2014 at 11:02:52AM +0100, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Stefan Hajnoczi <stefanha@redhat.com> writes:
> >
> >> The QEMU process stays running if the test case fails.  This patch fixes
> >> the leak by installing a SIGABRT signal handler which invokes
> >> qtest_end().
> >>
> >> In order to make that work for assertion failures during qtest_init(),
> >> we need to initialize QTestState fields including file descriptors and
> >> pids carefully.  qtest_quit() is then safe to call even during
> >> qtest_init().
> >>
> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> ---
> >>  tests/libqtest.c | 29 ++++++++++++++++++++++++++---
> >>  1 file changed, 26 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tests/libqtest.c b/tests/libqtest.c
> >> index 8b2b2d7..09a0481 100644
> >> --- a/tests/libqtest.c
> >> +++ b/tests/libqtest.c
> >> @@ -44,6 +44,7 @@ struct QTestState
> >>      bool irq_level[MAX_IRQ];
> >>      GString *rx;
> >>      pid_t qemu_pid;  /* our child QEMU process */
> >> +    struct sigaction sigact_old; /* restored on exit */
> >>  };
> >>  
> >>  #define g_assert_no_errno(ret) do { \
> >> @@ -88,6 +89,11 @@ static int socket_accept(int sock)
> >>      return ret;
> >>  }
> >>  
> >> +static void sigabrt_handler(int signo)
> >> +{
> >> +    qtest_end();
> 
> Don't you have to re-raise SIGABRT here, to actually terminate the
> process?

No.  POSIX says:

  RETURN VALUE
       The abort() function shall not return.

(BTW the way to avoid that is using longjmp.)

The Linux man page is more explicit:

  If  the  SIGABRT  signal  is ignored, or caught by a handler that returns,
  the abort() function will still terminate the process.  It does this by
  restoring the default disposition for  SIGABRT and then raising the signal
  for a second time.

Re: [Qemu-devel] [PATCH 3/3] qtest: kill QEMU process on g_assert() failure

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Tue, Feb 18, 2014 at 11:02:52AM +0100, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > Stefan Hajnoczi <stefanha@redhat.com> writes:
>> >
>> >> The QEMU process stays running if the test case fails.  This patch fixes
>> >> the leak by installing a SIGABRT signal handler which invokes
>> >> qtest_end().
>> >>
>> >> In order to make that work for assertion failures during qtest_init(),
>> >> we need to initialize QTestState fields including file descriptors and
>> >> pids carefully.  qtest_quit() is then safe to call even during
>> >> qtest_init().
>> >>
>> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> >> ---
>> >>  tests/libqtest.c | 29 ++++++++++++++++++++++++++---
>> >>  1 file changed, 26 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> >> index 8b2b2d7..09a0481 100644
>> >> --- a/tests/libqtest.c
>> >> +++ b/tests/libqtest.c
>> >> @@ -44,6 +44,7 @@ struct QTestState
>> >>      bool irq_level[MAX_IRQ];
>> >>      GString *rx;
>> >>      pid_t qemu_pid;  /* our child QEMU process */
>> >> +    struct sigaction sigact_old; /* restored on exit */
>> >>  };
>> >>  
>> >>  #define g_assert_no_errno(ret) do { \
>> >> @@ -88,6 +89,11 @@ static int socket_accept(int sock)
>> >>      return ret;
>> >>  }
>> >>  
>> >> +static void sigabrt_handler(int signo)
>> >> +{
>> >> +    qtest_end();
>> 
>> Don't you have to re-raise SIGABRT here, to actually terminate the
>> process?
>
> No.  POSIX says:
>
>   RETURN VALUE
>        The abort() function shall not return.
>
> (BTW the way to avoid that is using longjmp.)
>
> The Linux man page is more explicit:
>
>   If  the  SIGABRT  signal  is ignored, or caught by a handler that returns,
>   the abort() function will still terminate the process.  It does this by
>   restoring the default disposition for  SIGABRT and then raising the signal
>   for a second time.

Learn something new :)  Thanks!