Serenity: Shell for loop can't be interrupted, and causes kernel crash

[nico]

1. Run for i in $(seq 1000) { sleep 1; ls }
2. Hit ctrl-c a few times (or keep it depressed)

Expected: Quits loop
Actual: Ignored, then triggers assert in kernel.

@alimpfard for first bug, @tomuta for second :)

Shell(22): Command "sleep 1" finished in 31 ms
Shell(22): Command "ls" finished in 10 ms
[Terminal(7:7)]: /dev/pts/0: VINTR pressed!
[Terminal(7:7)]: /dev/pts/0: Send signal 2 to everyone in pgrp 85
[Terminal(7:7)]: /dev/pts/0: Send signal 2 to sleep(85)
Shell(22): Command "sleep 1" finished in 44 ms
Shell(22): Command "ls" finished in 13 ms
[Terminal(7:7)]: /dev/pts/0: VINTR pressed!
[Terminal(7:7)]: /dev/pts/0: Send signal 2 to everyone in pgrp 87
[Terminal(7:7)]: /dev/pts/0: Send signal 2 to Shell(87)
[Terminal(7:7)]: ASSERTION FAILED: Kernel::is_user_range(VirtualAddress(dest_ptr), n)
../Kernel/StdLib.cpp:47 in void copy_to_user(void*, const void*, size_t)
[Terminal(7:7)]: 0xc011eaed  __assertion_failed(char const*, char const*, unsigned int, char const*) +185
[Terminal(7:7)]: 0xc01677d0  copy_to_user +111
[Terminal(7:7)]: 0xc018192e  Kernel::push_value_on_user_stack(unsigned int*, unsigned int) +42
[Terminal(7:7)]: 0xc0182ce5  .L321 +239
[Terminal(7:7)]: 0xc0182f3d  Kernel::Thread::dispatch_one_pending_signal() +147
[Terminal(7:7)]: 0xc01659d1  Kernel::Thread::for_each_living<Kernel::Scheduler::pick_next()::{lambda(Kernel::Thread&)#4}>(Kernel::Scheduler::pick_next()::{lambda(Kernel::Thread&)#4})::{lambda(Kernel::Thread&)#1}::operator()(Kernel::Thread&) const [clone .isra.0] +93
[Terminal(7:7)]: 0xc01663b0  Kernel::Scheduler::pick_next() +996
[Terminal(7:7)]: 0xc0166775  Kernel::Scheduler::yield() +109
[Terminal(7:7)]: 0xc0181bee  Kernel::Thread::yield_without_holding_big_lock() +36
[Terminal(7:7)]: 0xc0176f27  Kernel::Process::sys$select(Kernel::Syscall::SC_select_params const*) +1173
[Terminal(7:7)]: 0xc01682bd  syscall_handler +1339
[Terminal(7:7)]: 0xc0167cf3  syscall_asm_entry +49
[Terminal(7:7)]: Spurious Interrupt, vector 15
[Terminal(7:7)]: Spurious Interrupt, vector 15
[Terminal(7:7)]: Spurious Interrupt, vector 15
[Terminal(7:7)]: Spurious Interrupt, vector 15
[Terminal(7:7)]: Spurious Interrupt, vector 15
[Terminal(7:7)]: Spurious Interrupt, vector 15
[Terminal(7:7)]: 0x080c8390  select +57
[Terminal(7:7)]: 0x080ad813  Core::EventLoop::wait_for_event(Core::EventLoop::WaitMode) +597
[Terminal(7:7)]: 0x080adb85  Core::EventLoop::pump(Core::EventLoop::WaitMode) +23
[Terminal(7:7)]: 0x080ade21  Core::EventLoop::exec() +43
[Terminal(7:7)]: 0x0804921f  main +3639
[Terminal(7:7)]: 0x08049c3e  _start +94

[alimpfard]

As far as I know, sleep quits with a zero when interrupted, so the interrupt will just look like the program exiting to the shell.

[alimpfard]

This patch:

diff --git a/Userland/sleep.cpp b/Userland/sleep.cpp
index 32dd64627..cec95af87 100644
--- a/Userland/sleep.cpp
+++ b/Userland/sleep.cpp
@@ -30,8 +30,11 @@
 #include <string.h>
 #include <unistd.h>

+static bool g_was_interrupted = false;
+
 static void handle_sigint(int)
 {
+    g_was_interrupted = true;
 }

 int main(int argc, char** argv)
@@ -56,5 +59,9 @@ int main(int argc, char** argv)
     if (remaining) {
         printf("Sleep interrupted with %u seconds remaining.\n", remaining);
     }
+
+    if (g_was_interrupted)
+        kill(getpid(), SIGKILL);
+
     return 0;
 }

along with my shell-jobs branch (#3372) does make the sadness go away.
The kernel issue / whatever the shell passes to select() issue is probably still viable though.

[nico]

Thanks, @tomuta!

Re sleep patch: It should be kill(getpid(), SIGINT), right? sleep just ignoring sigint does look incorrect.

[alimpfard]

SIGINT is handled and ignored, no?
what would re-raising the signal do for us?

[nico]

How ctrl-c is handled in shells / programs I know (I don't know if this is true yet in serenity's shell):

• hitting ctrl-c sends SIGINT to all the processes in the foreground process group
• some programs (say, matlab or vim) might chose to completely handle sigint, in this case the shell shouldn't quit the current script (if any)
• most programs just die on SIGINT through the default handler. here, the shell should quit any currently executing scripts

The way this is usually implemented is that the shell waits for its children, and if it receives a sigint it checks if the children died "as-if" the default SIGINT handler ran (WIFSIGNALED(rc) && WTERMSIG(rc) == SIGINT) then it aborts currently running scripts, else it doesn't.

See also https://github.com/nico/hack/blob/master/sigint.sh :)

[alimpfard]

Hmmm...so you're saying that the shell is in the same process group as whatever it spawns?
that sounds like a fragile thing to do...
currently, the shell puts every foreground process in a new (or if it's in a pipeline, that pipeline's) process group (unless it's in a subshell, since #3372)

leaving that aside, I do like restoring the default handler and re-raising the signal better; I'll do that with sleep

update: why would you mention _matlab_ of all things?

[nico]

Oh hm, you're right, the shell is probably in its own process group. I do not know if it is, but that would make sense to me. But if a child exits as-if interrupted, it still treats that as if it was interrupted itself -- that part still makes sense, right?

[alimpfard]

it sorta makes sense, but the shell signaling itself (or acting as if it was signaled) seems odd (instead of just using the information it already has in the proper places)

[nico]

Well yes the shell wouldn't signal itself of course. But for this to work, sleep needs to exit as if it was interrupted, which it currently doesn't.

[alimpfard]

well, sleep can be changed to do so
_"If only software was malleable"_

[nico]

Hey looks like this doesn't need any shell changes, https://github.com/SerenityOS/serenity/pull/3438 is enough. (Which might be what you expected, but to me it sounded like you were saying we might need shell changes maybe.) Hooray :)

[nico]

I take that back. It works for for i in $(seq 1000) { sleep 1 } but not for for i in $(seq 1000) { sleep 1; ls }, that's still un-interruptible.

[alimpfard]

3372 fixes that~

[nico]

With #3372 in, all problems here are fixed \o/