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

[Markus Armbruster]

Stefan Hajnoczi <address@hidden> writes:

> On Mon, Feb 17, 2014 at 06:00:03PM +0100, Markus Armbruster wrote:
>> Paolo Bonzini <address@hidden> 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.