Date: Mon, 7 Jun 2010 01:17:34 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Kostik Belousov <kostikbel@gmail.com> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Gabor Kovesdan <gabor@FreeBSD.org> Subject: Re: svn commit: r208868 - in head/usr.bin: bc dc Message-ID: <20100607004046.C30264@delplex.bde.org> In-Reply-To: <20100606120004.GH83316@deviant.kiev.zoral.com.ua> References: <201006061136.o56Ba9tr029717@svn.freebsd.org> <20100606120004.GH83316@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 6 Jun 2010, Kostik Belousov wrote:
> On Sun, Jun 06, 2010 at 11:36:09AM +0000, Gabor Kovesdan wrote:
>> @@ -223,14 +222,11 @@ static const struct jump_entry jump_tabl
>> (sizeof(jump_table_data)/sizeof(jump_table_data[0]))
>>
>> static void
>> -sighandler(int ignored)
>> +got_sigint(int ignored __unused)
>> {
>>
>> - switch (ignored)
>> - {
>> - default:
>> - bmachine.interrupted = true;
>> - }
>> + putchar('\n');
>> + exit(0);
>> }
> In general, calling not async-signal safe functions from the signal
> handler is an invitation for undefined behaviour, that usually manifests
> itself as SIGSEGV and SIGBUS.
It's always undefined, but usually has a null manifestation.
Both the putchar() and the exit() are highly async-signal-unsafe. A
correct signal handler would call _exit(nonzero) or kill itself to get
get the signal status in the exit code, but then the putchar() would
probably only write to the stdiio buffer.
The old code attempted to deal with this by using the usual hack of
only setting a variable of type sig_atomic_t (bmachine.interrupted)
in the interrupt handler, but that usually doesn't work on any BSDish
system since BSDish systems by default restart most syscalls after an
interrupt, so if the syscall is waiting for user input, it will be
restarted after a user SIGINT and resume waiting.
top(1) is broken in FreeBSD in the same way that dc was, while in the
vendor version it is broken in theory but rarely in practice by the
undefined behaviour. Try this: start top and enter command mode,
e.g. by "s<Enter>. Now ^C at the prompt appears not to work. But
a newline (possibly preceded by other input) makes the old ^C work
unexpectedly. This mostly works in the vendor version because the
signal handler is unsafe and just exits. Someone "fixed" this in
FreeBSD using the flag hack, without adding the (possibly very large)
complications needed to make the flag hack actually work.
To make the hack work you must either:
- turn off SA_RESTART for most signal actions. Then deal with most
syscalls possibly failing with errno = EINTR. Code portable to
old non-BSD systems need complications to deal with these EINTRS.
Now POSIX with XSI extensions in 2001 (maybe standard now) has
SA_RESTART, so a not-so-portable portable application can turn on
SA_RESTART so as not to have to deal with the EINTRS, and have
the BSD problems instead.
- locate all syscalls for i/o on interactive devices and other syscalls
of interest, and turn off SA_RESTART only while making these syscalls
and deal with the resulting EINTRs only for these syscalls. This is
probably easier than a global turnoff of SA_RESTART. This is probably
practical in a small application like top or dc (there seems to be only
1 critical read() syscall in top), but probably impossible in a large
application and difficult in one where the i/o is in libraries.
>From the original commit mail:
% Modified: head/usr.bin/bc/scan.l
% ==============================================================================
% --- head/usr.bin/bc/scan.l Sun Jun 6 11:32:38 2010 (r208867)
% +++ head/usr.bin/bc/scan.l Sun Jun 6 11:36:08 2010 (r208868)
% @@ -235,22 +234,6 @@ add_str(const char *str)
% strlcat(strbuf, str, strbuf_sz);
% }
%
% -/* ARGSUSED */
% -void
% -abort_line(int sig)
% -{
% - static const char str[] = "[\n]P\n";
% - int save_errno;
% -
% - switch (sig) {
% - default:
% - save_errno = errno;
% - YY_FLUSH_BUFFER; /* XXX signal race? */
% - write(STDOUT_FILENO, str, sizeof(str) - 1);
% - errno = save_errno;
% - }
% -}
This apparently tried to be async-signal safe by using write() instead of
stdio, but it isn't clear that this would mesh properly with i/o done
before or after the signal. More seriously, it calls YY_FLUSH_BUFFER,
which could do anything and is unlikeely to be async-signal safe.
% Modified: head/usr.bin/dc/bcode.c
% ==============================================================================
% --- head/usr.bin/dc/bcode.c Sun Jun 6 11:32:38 2010 (r208867)
% +++ head/usr.bin/dc/bcode.c Sun Jun 6 11:36:08 2010 (r208868)
% ...
% @@ -1746,14 +1742,6 @@ eval(void)
% bmachine.readsp--;
% continue;
% }
% - if (bmachine.interrupted) {
% - if (bmachine.readsp > 0) {
% - src_free();
% - bmachine.readsp--;
% - continue;
The old code seems to do further cleanup after a signal. This might be
more important than printing a newline.
% - } else
% - bmachine.interrupted = false;
% - }
% #ifdef DEBUGGING
% fprintf(stderr, "# %c\n", ch);
% stack_print(stderr, &bmachine.stack, "* ",
%
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100607004046.C30264>