[Qemu-devel] audit needed for signal handlers

[Qemu-devel] audit needed for signal handlers

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1747 bytes --]

Quick - identify the bug in this code (from ui/curses.c):

static void curses_winch_handler(int signum)
{
    struct winsize {
        unsigned short ws_row;
        unsigned short ws_col;
        unsigned short ws_xpixel;   /* unused */
        unsigned short ws_ypixel;   /* unused */
    } ws;

    /* terminal size changed */
    if (ioctl(1, TIOCGWINSZ, &ws) == -1)
        return;

    resize_term(ws.ws_row, ws.ws_col);
    curses_calc_pad();
    invalidate = 1;

    /* some systems require this */
    signal(SIGWINCH, curses_winch_handler);
}

Here's a hint: ioctl() can clobber errno.  But if a signal handler is
called in the middle of other code that is using errno, then the handler
MUST restore the value of errno before returning, if it is to guarantee
that the interrupted context won't be corrupted.

More reading on the topic:
https://plus.google.com/+LennartPoetteringTheOneAndOnly/posts/gHSscCJkakd

I have not done a full audit of qemu's signal handlers, so much as a
quick look to see if I could find violations; it was surprisingly easy
to find a bad example.  A signal handler that resets the signal to
SIG_DFL then calls raise() is exempt from caring about errno, but any
signal handler that can fall through to the end and return execution to
the caller MUST ensure that errno is left unchanged, for errno to be
useful in the remaining body of code.  Which is why the best signal
handlers tend to be the one that only flag a volatile variable that is
later checked at safe points of execution, rather than trying to make
complex calls from within the handler context.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] audit needed for signal handlers

[Anthony Liguori]

On Mon, Nov 11, 2013 at 8:50 AM, Eric Blake <eblake@redhat.com> wrote:
> Quick - identify the bug in this code (from ui/curses.c):
>
> static void curses_winch_handler(int signum)
> {
>     struct winsize {
>         unsigned short ws_row;
>         unsigned short ws_col;
>         unsigned short ws_xpixel;   /* unused */
>         unsigned short ws_ypixel;   /* unused */
>     } ws;
>
>     /* terminal size changed */
>     if (ioctl(1, TIOCGWINSZ, &ws) == -1)
>         return;
>
>     resize_term(ws.ws_row, ws.ws_col);
>     curses_calc_pad();
>     invalidate = 1;
>
>     /* some systems require this */
>     signal(SIGWINCH, curses_winch_handler);
> }
>
> Here's a hint: ioctl() can clobber errno.  But if a signal handler is
> called in the middle of other code that is using errno, then the handler
> MUST restore the value of errno before returning, if it is to guarantee
> that the interrupted context won't be corrupted.

Isn't this precisely why EINTR exists?

Regards,

Anthony Liguori

>
> More reading on the topic:
> https://plus.google.com/+LennartPoetteringTheOneAndOnly/posts/gHSscCJkakd
>
> I have not done a full audit of qemu's signal handlers, so much as a
> quick look to see if I could find violations; it was surprisingly easy
> to find a bad example.  A signal handler that resets the signal to
> SIG_DFL then calls raise() is exempt from caring about errno, but any
> signal handler that can fall through to the end and return execution to
> the caller MUST ensure that errno is left unchanged, for errno to be
> useful in the remaining body of code.  Which is why the best signal
> handlers tend to be the one that only flag a volatile variable that is
> later checked at safe points of execution, rather than trying to make
> complex calls from within the handler context.
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

Re: [Qemu-devel] audit needed for signal handlers

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]

On 11/11/2013 09:56 AM, Anthony Liguori wrote:

>> Here's a hint: ioctl() can clobber errno.  But if a signal handler is
>> called in the middle of other code that is using errno, then the handler
>> MUST restore the value of errno before returning, if it is to guarantee
>> that the interrupted context won't be corrupted.
> 
> Isn't this precisely why EINTR exists?

That's part of the equation, but not everything.  EINTR exists for a
system call that was cut short by the delivery of a signal; if you check
for errno==EINTR after a call that is documented to support it (such as
write() or poll()), then you know that the call was interrupted; use of
SA_RESTART with sigaction() can also control whether you will even see
EINTR in the first place for some functions.

But consider what happens when the system call completes normally, and
the signal handler then gets invoked in between the syscall completion
and the later code that checks the value of errno.  There, errno will
NOT be EINTR, and it is vital that the signal handler not corrupt errno
prior to returning control to normal execution context.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] audit needed for signal handlers

[Paolo Bonzini]

Il 11/11/2013 17:56, Anthony Liguori ha scritto:
> On Mon, Nov 11, 2013 at 8:50 AM, Eric Blake <eblake@redhat.com> wrote:
>> Quick - identify the bug in this code (from ui/curses.c):
>>
>> static void curses_winch_handler(int signum)
>> {
>>     struct winsize {
>>         unsigned short ws_row;
>>         unsigned short ws_col;
>>         unsigned short ws_xpixel;   /* unused */
>>         unsigned short ws_ypixel;   /* unused */
>>     } ws;
>>
>>     /* terminal size changed */
>>     if (ioctl(1, TIOCGWINSZ, &ws) == -1)
>>         return;
>>
>>     resize_term(ws.ws_row, ws.ws_col);
>>     curses_calc_pad();
>>     invalidate = 1;
>>
>>     /* some systems require this */
>>     signal(SIGWINCH, curses_winch_handler);
>> }
>>
>> Here's a hint: ioctl() can clobber errno.  But if a signal handler is
>> called in the middle of other code that is using errno, then the handler
>> MUST restore the value of errno before returning, if it is to guarantee
>> that the interrupted context won't be corrupted.
> 
> Isn't this precisely why EINTR exists?

No.

    do {
        rc = read(...);
    } while (rc == -1 && errno == EINTR);
    /* signal handler runs here */
    if (errno == EAGAIN) {
        ...
    }

That said, aren't all signals in QEMU (except SIG_IPI) caught with
signalfd and the handlers run synchronously in the iothread?

Paolo

Re: [Qemu-devel] audit needed for signal handlers

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

On 11/11/2013 10:05 AM, Paolo Bonzini wrote:

> 
> That said, aren't all signals in QEMU (except SIG_IPI) caught with
> signalfd and the handlers run synchronously in the iothread?

signalfd is currently a Linux-only concept - what happens on BSD?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] audit needed for signal handlers

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 11/11/2013 18:08, Eric Blake ha scritto:
>>> That said, aren't all signals in QEMU (except SIG_IPI) caught
>>> with signalfd and the handlers run synchronously in the
>>> iothread?
> signalfd is currently a Linux-only concept - what happens on BSD?

It is emulated with a thread and sigwait (see compatfd.c).

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSgQ+pAAoJEBvWZb6bTYbyC1YQAIleHFtvyjlVzmY5MxHKN3L2
YkCLE3s/8dRRvH1IjD61LA3RxJHFgBK6+i45bw8Am+ydRVoCLJ3/tyPqP1ygyg/q
w/lhITmpe4h0w8W4omI3RHh3+Ueh3Slcobsrzc1js197Dy4eL2RLdfvtIVrbe8Qj
cPz7hML+2sGv6zqL7mebdVRjxqKNMoVXyLuuFGndAc7shSGJ8XLl4Cn/b46Efl9z
zjNzCxd0j2FJ3mcoINZ1ON2pv73pwqhDCaIVZFSMwOhuY09Rh1rT9tJ+pbXI1ZlU
pz6ThYcnxJSTqWOqVm6GJxA0bgec0ZejZkPYALgn7dWRIL9iLpr5FmxDpg69+me3
p6ZoZbF1QI5BzndEJw97/4S9cumxkPVo1KPkJAPA3uQfta/L9l2q4PReMbHj3qTS
q35wCOI9se/JkiB/9872WvXi9b6OSxTQEXFpDspglojRgd96yuWVI2aBeUtPLKJh
7fShY4JRU41cC1cp9gdK/dv1wFCs6D6U5mPnsN0t0hYboZiIwLusGrhSagWf34aK
Fg829GgKhebE/YD/lupMIfqEPb3UxkK7y/ME27789eiP81j/ckuXBI++/yPh5iIx
dXJx0nD8qJApjKePjJGJYRXGIB9MS/GTi2mdQltVkcr74CLaoPFe2epFhB04zmC2
k4oCTb/TZ78+Eh/oHt7p
=c3gn
-----END PGP SIGNATURE-----

Re: [Qemu-devel] audit needed for signal handlers

[Peter Maydell]

On 11 November 2013 16:56, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On Mon, Nov 11, 2013 at 8:50 AM, Eric Blake <eblake@redhat.com> wrote:
>> Here's a hint: ioctl() can clobber errno.  But if a signal handler is
>> called in the middle of other code that is using errno, then the handler
>> MUST restore the value of errno before returning, if it is to guarantee
>> that the interrupted context won't be corrupted.
>
> Isn't this precisely why EINTR exists?

EINTR won't help you in the case like:

   ret = some_syscall();
   [execution returns from syscall, with errno set to X]
   [signal happens here; handler trashes errno]
   if (ret < 0) {
      use errno;
   }

EINTR exists mostly because properly resuming syscalls
was too hard for Bell Labs :-)

-- PMM

Re: [Qemu-devel] audit needed for signal handlers

[Peter Maydell]

On 11 November 2013 17:05, Paolo Bonzini <pbonzini@redhat.com> wrote:
> That said, aren't all signals in QEMU (except SIG_IPI) caught with
> signalfd and the handlers run synchronously in the iothread?

Eric specifically points out one which is not.
(I'm pretty sure that 'reinstall signal handler at
end of signal handler' is ancient voodoo that we don't
want either, incidentally.)

-- PMM

Re: [Qemu-devel] audit needed for signal handlers

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]

On 11/11/2013 10:13 AM, Peter Maydell wrote:
> On 11 November 2013 17:05, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> That said, aren't all signals in QEMU (except SIG_IPI) caught with
>> signalfd and the handlers run synchronously in the iothread?
> 
> Eric specifically points out one which is not.
> (I'm pretty sure that 'reinstall signal handler at
> end of signal handler' is ancient voodoo that we don't
> want either, incidentally.)

Reinstalling the signal handler is voodoo needed only for systems where
SA_RESETHAND is the default behavior of signal() (POSIX says that the
older signal() is implementation-defined whether it behaves like
SA_RESETHAND|SA_NODEFER [SysV] or SA_RESTART [BSD] - so it is already
mandatory to use sigaction() if you don't want to be bitten by the
difference in semantics; but if you can assume working sigaction(), then
don't use signal() or SA_RESETHAND in the first place, and you don't
need the reinstall voodoo in your handlers).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] audit needed for signal handlers

[Paolo Bonzini]

Il 11/11/2013 18:13, Peter Maydell ha scritto:
>> > That said, aren't all signals in QEMU (except SIG_IPI) caught with
>> > signalfd and the handlers run synchronously in the iothread?
> Eric specifically points out one which is not.
> (I'm pretty sure that 'reinstall signal handler at
> end of signal handler' is ancient voodoo that we don't
> want either, incidentally.)

Yeah, I was convinced it was---I still cannot find a reason why SIGWINCH
needs to be handled synchronously.

resize_term is definitely not signal safe; the man page reflects
10-year-old (or more) signal handling lore: "While these functions are
intended to be used to support a signal handler (i.e., for SIGWINCH),
care should be taken to avoid invoking them in a context where malloc or
realloc may have been interrupted, since it uses those functions".

Calling malloc/realloc from a signal handler is taboo these days...

Paolo

Re: [Qemu-devel] audit needed for signal handlers

[Max Filippov]

On Mon, Nov 11, 2013 at 8:50 PM, Eric Blake <eblake@redhat.com> wrote:
> Quick - identify the bug in this code (from ui/curses.c):
>
> static void curses_winch_handler(int signum)
> {
>     struct winsize {
>         unsigned short ws_row;
>         unsigned short ws_col;
>         unsigned short ws_xpixel;   /* unused */
>         unsigned short ws_ypixel;   /* unused */
>     } ws;
>
>     /* terminal size changed */
>     if (ioctl(1, TIOCGWINSZ, &ws) == -1)

An unsafe function is called in a signal. See man 7 signal,
section 'Async-signal-safe functions'. This should be avoided.

>         return;
>
>     resize_term(ws.ws_row, ws.ws_col);
>     curses_calc_pad();
>     invalidate = 1;
>
>     /* some systems require this */
>     signal(SIGWINCH, curses_winch_handler);
> }
>
> Here's a hint: ioctl() can clobber errno.

I believe it cannot, at least in linux, as technically the signal
handler is always called in a new thread, specifically created
to only handle that signal, and errno should be thread-local.

>  But if a signal handler is
> called in the middle of other code that is using errno, then the handler
> MUST restore the value of errno before returning, if it is to guarantee
> that the interrupted context won't be corrupted.
>
> More reading on the topic:
> https://plus.google.com/+LennartPoetteringTheOneAndOnly/posts/gHSscCJkakd
>
> I have not done a full audit of qemu's signal handlers, so much as a
> quick look to see if I could find violations; it was surprisingly easy
> to find a bad example.  A signal handler that resets the signal to
> SIG_DFL then calls raise() is exempt from caring about errno, but any
> signal handler that can fall through to the end and return execution to
> the caller MUST ensure that errno is left unchanged, for errno to be
> useful in the remaining body of code.  Which is why the best signal
> handlers tend to be the one that only flag a volatile variable that is
> later checked at safe points of execution, rather than trying to make
> complex calls from within the handler context.

-- 
Thanks.
-- Max

Re: [Qemu-devel] audit needed for signal handlers

[Gerd Hoffmann]

On Mo, 2013-11-11 at 18:47 +0100, Paolo Bonzini wrote:
> Il 11/11/2013 18:13, Peter Maydell ha scritto:
> >> > That said, aren't all signals in QEMU (except SIG_IPI) caught with
> >> > signalfd and the handlers run synchronously in the iothread?
> > Eric specifically points out one which is not.
> > (I'm pretty sure that 'reinstall signal handler at
> > end of signal handler' is ancient voodoo that we don't
> > want either, incidentally.)
> 
> Yeah, I was convinced it was---I still cannot find a reason why SIGWINCH
> needs to be handled synchronously.

There is zero need.  And changing that is actually the correct fix IMHO:
Just set a flag in the signal handler (i.e. no syscalls which then could
corrupt errno), then handle it the next time we update the screen.

cheers,
  Gerd

Re: [Qemu-devel] audit needed for signal handlers

[Laszlo Ersek]

On 11/11/13 18:47, Paolo Bonzini wrote:
> Il 11/11/2013 18:13, Peter Maydell ha scritto:
>>>> That said, aren't all signals in QEMU (except SIG_IPI) caught with
>>>> signalfd and the handlers run synchronously in the iothread?
>> Eric specifically points out one which is not.
>> (I'm pretty sure that 'reinstall signal handler at
>> end of signal handler' is ancient voodoo that we don't
>> want either, incidentally.)
> 
> Yeah, I was convinced it was---I still cannot find a reason why SIGWINCH
> needs to be handled synchronously.
> 
> resize_term is definitely not signal safe; the man page reflects
> 10-year-old (or more) signal handling lore: "While these functions are
> intended to be used to support a signal handler (i.e., for SIGWINCH),
> care should be taken to avoid invoking them in a context where malloc or
> realloc may have been interrupted, since it uses those functions".
> 
> Calling malloc/realloc from a signal handler is taboo these days...

The problem is when you call a non-async-signal-safe from the handler
and you've interrupted another non-async-signal-safe function. Ie.
there's trouble *only* if both interruptor and interruptee are
non-async-signal-safe.

If at least one is async-signal-safe, then there's no problem. The more
frequent case for this is when the handler is async-signal-safe (like
when it sets a volatile sig_atomic_t flag, and/or uses local variables
exclusively and possibly calls other async-signal-safe functions).

But, the reverse setup exists as well, when you do whatever you like in
the signal handler, just restrict it (ie. the signal delivery) to
portions of the "normal" source where no non-async-signal-safe functions
can run. One example is when you block the signal for the entire event
loop body, and you unblock it only atomically in pselect() or ppoll().
If there has been a signal pending, the (non-async-signal-safe) handler
body will run *inside* pselect(), but that's fine.

(Of course you still need to care about *other* signals interrupting the
handler, see also sa_mask.)

It's by far the best to do what Eric suggests (and I usually combine
that even with the "block it everywhere" idea, because I hate EINTR --
in a correct event loop no function call should block anyway, and EINTR
is just a nuisance).

I probably glossed over a thousand details and I'll get shredded for
that, but that's OK. :)

Done ranting :)

Thanks,
Laszlo

Re: [Qemu-devel] audit needed for signal handlers

[Laszlo Ersek]

On 11/11/13 19:03, Max Filippov wrote:
> On Mon, Nov 11, 2013 at 8:50 PM, Eric Blake <eblake@redhat.com> wrote:
>> Quick - identify the bug in this code (from ui/curses.c):
>>
>> static void curses_winch_handler(int signum)
>> {
>>     struct winsize {
>>         unsigned short ws_row;
>>         unsigned short ws_col;
>>         unsigned short ws_xpixel;   /* unused */
>>         unsigned short ws_ypixel;   /* unused */
>>     } ws;
>>
>>     /* terminal size changed */
>>     if (ioctl(1, TIOCGWINSZ, &ws) == -1)
> 
> An unsafe function is called in a signal. See man 7 signal,
> section 'Async-signal-safe functions'. This should be avoided.
> 
>>         return;
>>
>>     resize_term(ws.ws_row, ws.ws_col);
>>     curses_calc_pad();
>>     invalidate = 1;
>>
>>     /* some systems require this */
>>     signal(SIGWINCH, curses_winch_handler);
>> }
>>
>> Here's a hint: ioctl() can clobber errno.
> 
> I believe it cannot, at least in linux, as technically the signal
> handler is always called in a new thread, specifically created
> to only handle that signal, and errno should be thread-local.

That's incorrect (*). The handler runs on a new *stack frame*, but
inside the same thread. You can specify an alternate stack for signal
handlers to run on in advance (see SA_ONSTACK / sigaltstack()), in which
case the new stack frame will be "allocated" there. Otherwise the system
will just use the normal stack. The handler indeed runs like an
"unexpected", "out-of-the-blue" normal function call.

It is actually pretty vital that the handler is run by the specific
thread that the signal has been delivered to.

(*) Example code (not a correct/portable program due to the race on
"errno", but it does disprove the idea that errno is "protected" on Linux):

  #define _XOPEN_SOURCE 600

  #include <unistd.h>
  #include <errno.h>
  #include <signal.h>
  #include <stdio.h>

  static void ringring(int signum)
  {
    errno = -12;
  }

  int main(void)
  {
    sigaction(SIGALRM,
              &(struct sigaction){ .sa_handler = &ringring },
              NULL);
    alarm(1);

    errno = 0;
    /* pause() would clobber errno */
    while (errno == 0)
      ;

    printf("%d\n", errno);
    return 0;
  }

Thanks
Laszlo