[Bug 3547] shell read is maybe too safe

[Denys Vlasenko]

On Mon, May 9, 2011 at 11:19 AM, Laurent Bercot
<ska-dietlibc at skarnet.org> wrote:
>> Although I think the fix revealed a deeper problem:
>> hush's way of handling signals isn't good for read
>> and for reading commands from stdin: since we block
>> signals we are interested in, we cannot detect them
>> while we wait for input on a fd.
>
>> Changing signal handling to a different scheme,
>> where they are caught and recorded, will somewhat help,
>> because now poll/read syscalls will be interruptible,
>> but this creates another problem: many other sycalls
>> may start failing with EINTR too, and we don't check
>> for that. The problem is, EINTR case happens rarely.
>> IOW: we may have many rarely-triggered bugs, one
>> per every syscall we don't wrap with "repeat on EINTR"
>> code, or with "if we got EINTR, propagate EINTR".
>
>  Indeed.
>  http://www.skarnet.org/tmp/doc/libstddjb/safewrappers.html
>  (non-permanent URL)
>  Safe wrappers should be used everywhere, *even* when SIGINT is
> meant to be detected: the handling part must be done in another
> place than the chunk of code containing your interrupted system
> call. See below.

> For instance, you cannot trap SIGSTOP; SIGSTOP does not kill your process, which should resume flawlessly at the next SIGCONT; and according to the specification, it is valid for SIGSTOP and SIGCONT to not have SA_RESTART behaviour.

I don't think that such SIGSTOP behavior isn't a kernel bug.
It would break a lot of programs, because no one stress tests
their apps with massive SIGSTOP/SIGCONT storms, so many apps
are bound to contain bugs wrt this.
Practicality dictates that the kernel-side fix is a better place for this
than auditing quadrazillion of syscalls in userspace code.

SIGCONT is just another signal - you can even have a handler for it.
Try it if you don't believe me - it works:

#include <errno.h>
#include <string.h>
#include <stdio.h>
#include <signal.h>
#include <unistd.h>
static void sig(int n)
{
      char buf[128];
      int e = errno;
      sprintf(buf, "sig: %d %s\n", n, strsignal(n));
      write(1, buf, strlen(buf));
      errno = e;
}
int main()
{
      signal(SIGSTOP, sig);
      signal(SIGCONT, sig);
      signal(SIGWINCH, sig);
      signal(SIGABRT, sig);
      printf("PID: %d\n", getpid());
      fflush(NULL);
      sleep(30);
      return 0;
}

$ gcc t.c
$ ./a.out
PID: 3770
<-- kill -STOP 3770
<-- kill -ABRT 3770
<-- kill -WINCH 3770
<-- kill -CONT 3770
sig: 28 Window changed
sig: 18 Continued  <========NOTE THIS!
sig: 6 Aborted


Because it's an "ordinary" signal (unlike SIGSTOP/SIGKILL),
you definitely can set SA_RESTART on SIGCONT.
The fact that SIGCONT wakes up SIGSTOPed task
is a special side effect which is orthogonal to SA_RESTARTness.


>> Ideas anyone?
>
>  As always when you need to mix signals and I/O: use a self-pipe.
>  http://www.skarnet.org/tmp/doc/libstddjb/selfpipe.html
>  (non-permanent URL)
>  and http://cr.yp.to/docs/selfpipe.html
>
>  Create a pipe p, trap SIGINT with a "write(p[1], &c, 1)" handler,
> and loop over a poll() on stdin and p[0] (and maybe other stuff you need).
>
> Use safe wrappers for your (asynchronous) reads and your (preferably
> asynchronous, but it doesn't really matter) writes.
>  When you get a SIGINT, it will be momentarily ignored if you're in the
> middle of performing some operation ; but there will be a byte in p, and
> as soon as you come back to the poll(), p[0] will be readable, you will
> know you got a SIGINT, and can handle it in a normal (i.e.
> non-interrupt-handler) context.
>
>  The self-pipe trick is the ultimate solution to mixing I/O and signals,
> because it integrates signal handling into the generic select/poll
> asynchronous I/O event framework. There is no case it cannot handle.
> Teaching it should be a mandatory part of every Unix course. :)

Yep, I know the technique.
But.
This is  *shell*. In shell, people may use fd's *directly*:

echo yo! >&23

What will happen if they'd write stuff to the write end
of the signal pipe?!

It is already a huge PITA to prevent users from seeing/reading from
the fd's opened to the currently running script! Think about this:

script.sh:
====
. ./foo.sh
foo.sh:
====
cat <&4
...

If script.sh is opened to fd 3 and foo.sh is opened to fd 4 by shell,
what will happen?


Therefore, signal pipe isn't an attractive solution.

Will it work if I'll set signal handlers to SA_RESTART, but perform
all reads by checking data availability via poll() beforehand?
The trick is that poll() is interrupted by SA_RESTART signals!
(Unlike read(), which isn't).

-- 
vda