search

ish-app / ish

Linux shell for iOS
https://ish.app
Other
16.91k stars 890 forks source link

Fix pty close behavior, signals at exit #2387

Closed cgull closed 4 months ago

cgull commented 4 months ago

There's two big things in here.

When running mosh --local 0, I found that mosh-server would never terminate on iSH. It was waiting for the master side of its pty to close. Most applications that use ptys use other methods to detect child termination, but mosh-server expects that the pty master will act as a closed file after the last program on the pty slave exits-- any waiting reads should return immediately with 0 bytes, and further read calls should return EOF. This is what pipes and sockets do, of course. iSH master ptys stayed in the read call forever, even though the pty slave was gone. This fixes that and ends up better separating generic tty code from pty slaves from pty masters too.

dtach behaves similarly and the fix helps it too.

After I fixed that, I found that CLI ish would exit with signal 9 when mosh-server exited. This is because the README recommends to use /bin/login as the starting process. Unfortunately, that means /bin/login runs as pid 1 (init) and is notified when orphaned processes terminate (which mosh-server is, because it double forks). /bin/login has never been intended as a process group leader or init process, it expects to start and wait() for exactly 1 child, usually an interactive shell.

But running ish -f alpine /bin/sh didn't work correctly either, when the initial shell exited iSH would exit on signal 9. (Or something, this was a couple of months ago and I'm forgetting details. Maybe it was still when mosh-server exited, or when a subshell exited.) That turned out to be a nasty problem with halt_system(). This code runs when pid 1 exits. This code uses pthread_kill() to send a SIGKILL to threads running the emulated process group to kill off emulated processes in iSH. Uhh... that doesn't work. SIGKILL always nukes the entire process, not a single thread.

After I changed that code to do a more kernel-like thing of using deliver_signal() to send first SIGTERM, then SIGKILL, and then finally pthread_kill() with SIGTERM, iSH behaves a lot better about emulated process termination, and exits cleanly (rather than with SIGKILL) when pid 1 terminates.

I changed the README to tell people to use /bin/sh as the starting process for CLI ish, because it can work as init/pid 1 and ignores termination of orphaned processes. iSH.app should probably change to do the same.

I've not been able to build iSH.app in XCode, so all of this is untested there, but I believe it will help there-- the app won't quit when the initial shell exits.

After fixing this and then finding/fixing the procfs problem in #2386, I ran into yet another bug in iSH, I forget what it was. At that point I gave up chasing bugs in iSH.

cgull commented 4 months ago

Tabs removed.

francoisvignon commented 4 months ago

Hi.

It seem the new behavior suffer an issue: once the client close his side of the pty, no new connection can be issued to the provider without closing and reopening the provider

An idea ?

Francois

Le 15 mai 2024 à 18:02, tbodt @.***> a écrit :

Merged #2387 https://github.com/ish-app/ish/pull/2387 into master.

— Reply to this email directly, view it on GitHub https://github.com/ish-app/ish/pull/2387#event-12820281343, or unsubscribe https://github.com/notifications/unsubscribe-auth/AK466UU3JOVMRSXAS5BBMOTZCOBIFAVCNFSM6AAAAABHRSHQV2VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJSHAZDAMRYGEZTIMY. You are receiving this because you are subscribed to this thread.

tbodt commented 4 months ago

@francoisvignon do you have an easy way to reproduce? @cgull could you check on this?

francoisvignon commented 4 months ago

here is an easy way to reproduce.

using iSH 669 and :

source: main.c

#define _GNU_SOURCE
#include <stdio.h>
#include <errno.h>
#include <fcntl.h>
#include <stdlib.h>
#include <unistd.h>

int main(int argv, char **argc)
{
    int fd;
    char buf[256];
    fd = posix_openpt(O_RDWR | O_NOCTTY);
    grantpt(fd);
    unlockpt(fd);
    ptsname_r(fd, buf, 256);
    printf("client name %s\n", buf);

    for(;;)
    {
        int l;
        l = read(fd, buf, 256);
        printf("l=%d, errno=%d\n", l, errno);
        sleep(1); // to not have too many traces ;-)
    }
    return(0);
}

compiling by : 

make main

executing:

McFv:~/test# ./main 
client name /dev/pts/2
// from other iSH window, connect to /dev/pts/2 by : minicom -D /dev/pts/2
// successfull
// keying CR one and two times
l=1, errno=0
l=2, errno=0
// disconnecting minicom by ^A ^X
l=-1, errno=5
l=0, errno=5
l=0, errno=5
l=0, errno=5
// from other iSH window, trying to reconnect by : minicom -D /dev/pts/2
// no way ...
l=0, errno=5
l=0, errno=5
l=0, errno=5
l=0, errno=s
// killing main
^C
McFv:
// and restarting main
McFv:~/test# ./main
client name /dev/pts/2 
// from other iSH window, connect to /dev/pts/2 by : minicom -D /dev/pts/2
// successfull
// keying CR one time
1=1, errno=0
// disconnecting minicom by ^A ^X
l=-1, errno=5
l=0, errno=5
l=0, errno=5
l=0, errno=5
l=0, errno=5
...

the same test with iSH 614

McFv:~/test# ./main
client name /dev/pts/2
// connect to /dev/pts/2 by : minicom -D /dev/pts/2
// successfull
// keying CR one and two times
l=1, errno=0
l=2, errno=0
// disconnecting minicom by ^A ^X
// read not interrrupted by disconnection, so, as read is blocking ... no new traces
// from other iSH window, trying to reconnect by : minicom -D /dev/pts/2
// successfull
// keying CR one and two times
l=1, errno=0
// killing main
^C
// minicom show /dev/pts/2 is not longer available
McFv:

with 614, client disconnection can be detected by other mean (setting read not blocking and checking errno).

please note, the source is voluntary simple to be the shortest code showing the issue (as example, displaying errno if not error is voluntary to not have too many lines, not setting read as not blocking also, ... ).

pelase note also: MacOs and Debian allows reconnection (like 614) but manage disconnection like 669 (but with a little variation about read returning -1 for debian and 0 for MacOs)

@cgull : I'm available to do more testing. thanks for your worlk

tbodt commented 4 months ago

Ah, one bug replaced by another... The behavior on linux is different from either of those.

$ ./main
client name /dev/pts/1
// connect, type stuff
l=1, errno=0
l=3, errno=0
l=1, errno=0
l=6, errno=0
l=6, errno=0
// disconnect, get error 5 repeating
l=-1, errno=5
l=-1, errno=5
l=-1, errno=5
l=-1, errno=5
// reconnect, errors stop (well it still says `errno=5` but since the actual return code was success ignore that)
l=1, errno=5
l=2, errno=5
// disconnect, errors are back
l=-1, errno=5
l=-1, errno=5
l=-1, errno=5
l=-1, errno=5
l=-1, errno=5
l=-1, errno=5
l=-1, errno=5
l=-1, errno=5
francoisvignon commented 4 months ago

this test was done with what ?

Francois

Le 18 mai 2024 à 17:13, tbodt @.***> a écrit :

Ah, one bug replaced by another... The behavior on linux is different from either of those.

$ ./main client name /dev/pts/1 // connect, type stuff l=1, errno=0 l=3, errno=0 l=1, errno=0 l=6, errno=0 l=6, errno=0 // disconnect, get error 5 repeating l=-1, errno=5 l=-1, errno=5 l=-1, errno=5 l=-1, errno=5 // reconnect, errors stop (well it still says errno=5 but since the actual return code was success ignore that) l=1, errno=5 l=2, errno=5 // disconnect, errors are back l=-1, errno=5 l=-1, errno=5 l=-1, errno=5 l=-1, errno=5 l=-1, errno=5 l=-1, errno=5 l=-1, errno=5 l=-1, errno=5 — Reply to this email directly, view it on GitHub https://github.com/ish-app/ish/pull/2387#issuecomment-2118853826, or unsubscribe https://github.com/notifications/unsubscribe-auth/AK466UUL6QH4WHPBJRKGXRDZC5VYTAVCNFSM6AAAAABHRSHQV2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJYHA2TGOBSGY. You are receiving this because you were mentioned.

tbodt commented 4 months ago

Linux linux-luka 6.7.6-1-aarch64-ARCH #1 SMP PREEMPT_DYNAMIC Sat Feb 24 10:08:35 MST 2024 aarch64 GNU/Linux

francoisvignon commented 4 months ago

ok, this is the "expected" behavior :-)

Francois

Le 18 mai 2024 à 17:41, tbodt @.***> a écrit :

Linux linux-luka 6.7.6-1-aarch64-ARCH #1 https://github.com/ish-app/ish/issues/1 SMP PREEMPT_DYNAMIC Sat Feb 24 10:08:35 MST 2024 aarch64 GNU/Linux

— Reply to this email directly, view it on GitHub https://github.com/ish-app/ish/pull/2387#issuecomment-2118861337, or unsubscribe https://github.com/notifications/unsubscribe-auth/AK466UXO5U5XFHIZYIFUF5DZC5ZBDAVCNFSM6AAAAABHRSHQV2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJYHA3DCMZTG4. You are receiving this because you were mentioned.

tbodt commented 4 months ago

I'm trying to decide if I should revert this PR, if the new bug is worse than the old bug. Obviously ideally both bugs would be fixed, but I don't really have time to do that work. How bad is the new behavior?

francoisvignon commented 4 months ago

I will check the old bug tomorrow: I have used the old pty implementation without major issue ... so, I will double check, and I tell you ...send from my mobile.____FrancoisLe 18 mai 2024 à 18:35, tbodt @.***> a écrit : I'm trying to decide if I should revert this PR, if the new bug is worse than the old bug. Obviously ideally both bugs would be fixed, but I don't really have time to do that work.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

tbodt commented 4 months ago

Do you run into major issues with the new behavior?

francoisvignon commented 4 months ago

Major ? annoying is a better word:- with the previous implementaion, I can check if a client is connected or not: with this one, not longer.- with this one, when the client disconnect, I close the master pty and open it again, with the risk to have a different client pty.So ... I prefer the old one (more standard accross various plateform) and I think the solution used to solve the issue solved by the new one is maybe not the correct one.send from my mobile.____FrancoisLe 18 mai 2024 à 18:50, tbodt @.***> a écrit : Do you run into major issues with the new behavior?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

cgull commented 4 weeks ago

I think I've fixed this in #2449. Sorry for dropping the ball on it. I'm curious, though, what sort of application would you be running in iSH that needs this? The things I can think of include embedded applications or applications needing to connect to or emulate physical devices, none of which seem like something you'd want to do in an iOS app.