[Bug 3547] shell read is maybe too safe
vda.linux at googlemail.com
Mon May 9 11:25:57 UTC 2011
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".
> (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:
static void sig(int n)
int e = errno;
sprintf(buf, "sig: %d %s\n", n, strsignal(n));
write(1, buf, strlen(buf));
errno = e;
printf("PID: %d\n", getpid());
$ gcc t.c
<-- 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.
> (non-permanent URL)
> and http://cr.yp.to/docs/selfpipe.html
> Create a pipe p, trap SIGINT with a "write(p, &c, 1)" handler,
> and loop over a poll() on stdin and p (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 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.
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:
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).
More information about the busybox