From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:48753)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1VgCyq-0003lV-Pr
	for qemu-devel@nongnu.org; Tue, 12 Nov 2013 07:22:16 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1VgCyk-0002nf-Oi
	for qemu-devel@nongnu.org; Tue, 12 Nov 2013 07:22:04 -0500
Received: from mx1.redhat.com ([209.132.183.28]:56806)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1VgCyk-0002nb-F9
	for qemu-devel@nongnu.org; Tue, 12 Nov 2013 07:21:58 -0500
Message-ID: <52821E08.8030508@redhat.com>
Date: Tue, 12 Nov 2013 13:24:40 +0100
From: Laszlo Ersek <lersek@redhat.com>
MIME-Version: 1.0
References: <52810AEA.5020806@redhat.com>
	<CAMo8Bf+jJuArKvjSTi5wj5HqWkYSVZwmY7YCctJrtOh1qK55Xg@mail.gmail.com>
In-Reply-To: <CAMo8Bf+jJuArKvjSTi5wj5HqWkYSVZwmY7YCctJrtOh1qK55Xg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] audit needed for signal handlers
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Max Filippov <jcmvbkbc@gmail.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>

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