[PATCH] os_unix: remove unnecessary process group stuff

[Felipe Contreras]

Processes are not being started correctly with TIOCSCTTY, which does
succeed after setsid().

None of that stuff is necessary, including the silencing of SIGHUP.

It's not even executed when shellpipe is off.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---
src/os_unix.c | 21 ---------------------
1 file changed, 21 deletions(-)

diff --git a/src/os_unix.c b/src/os_unix.c
index 20c61106f..89d37740e 100644
--- a/src/os_unix.c
+++ b/src/os_unix.c
@@ -4753,33 +4753,12 @@ mch_call_shell_fork(
)
{

-# ifdef HAVE_SETSID
- // Create our own process group, so that the child and all its
- // children can be kill()ed. Don't do this when using pipes,
- // because stdin is not a tty, we would lose /dev/tty.
- if (p_stmp)
- {
- (void)setsid();
-# if defined(SIGHUP)
- // When doing "!xterm&" and 'shell' is bash: the shell
- // will exit and send SIGHUP to all processes in its
- // group, killing the just started process. Ignore SIGHUP
- // to avoid that. (suggested by Simon Schubert)
- signal(SIGHUP, SIG_IGN);
-# endif
- }
-# endif
# ifdef FEAT_GUI
if (pty_slave_fd >= 0)
{
// push stream discipline modules
if (options & SHELL_COOKED)
setup_slavepty(pty_slave_fd);
-# ifdef TIOCSCTTY
- // Try to become controlling tty (probably doesn't work,
- // unless run by root)
- ioctl(pty_slave_fd, TIOCSCTTY, (char *)NULL);
-# endif
}
# endif
set_default_child_environment(FALSE);
--
2.32.0.rc0

[Felipe Contreras]

On Mon, May 31, 2021 at 2:22 AM Felipe Contreras
<felipe.c...@gmail.com> wrote:
>
> Processes are not being started correctly with TIOCSCTTY, which does
> succeed after setsid().

I'm using the following program that uses posix_spawn as a test.

When using gvim, shelltemp on, and running it with :! nothing happens.

#include <spawn.h>
#include <signal.h>

extern char **environ;

int main(void)
{
char *const argv[] = { "/usr/bin/chromium", "http://google.com", NULL };
posix_spawnattr_t attr;
sigset_t mask;

posix_spawnattr_init(&attr);
sigemptyset(&mask);
sigaddset(&mask, SIGHUP);

posix_spawnattr_setsigdefault(&attr, &mask);
posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETSIGDEF);

return posix_spawn(NULL, argv[0], NULL, &attr, argv, environ);
}

--
Felipe Contreras

[Bram Moolenaar]

Felipe Contreras wrote:

> Processes are not being started correctly with TIOCSCTTY, which does
> succeed after setsid().
>
> None of that stuff is necessary, including the silencing of SIGHUP.
>
> It's not even executed when shellpipe is off.

I suspect removing setsid() will cause trouble if a terminal with a
shell is started and that shell exits. There is a reason this code was
added. It was done long ago, I don't recall how to reproduce the
problem.

Just removing the TIOCSCTTY seems to already fix the problem. So how
about just deleting these lines:

>
> -# ifdef TIOCSCTTY
> - // Try to become controlling tty (probably doesn't work,
> - // unless run by root)
> - ioctl(pty_slave_fd, TIOCSCTTY, (char *)NULL);
> -# endif

I wonder what the simplest way is to test the fix.


--
ROBIN: The what?
ARTHUR: The Holy Hand Grenade of Antioch. 'Tis one of the sacred relics
Brother Maynard always carries with him.
ALL: Yes. Of course.
ARTHUR: (shouting) Bring up the Holy Hand Grenade!
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[Felipe Contreras]

On Mon, May 31, 2021 at 12:23 PM Bram Moolenaar <Br...@moolenaar.net> wrote:
> Felipe Contreras wrote:
>
> > Processes are not being started correctly with TIOCSCTTY, which does
> > succeed after setsid().
> >
> > None of that stuff is necessary, including the silencing of SIGHUP.
> >
> > It's not even executed when shellpipe is off.
>
> I suspect removing setsid() will cause trouble if a terminal with a
> shell is started and that shell exits.

No, I tested that.

Moreover, this is only done with the GUI *and* `shellpipe` is disabled.

If there was a problem with terminating shells, shouldn't we already
see the problem on console, or when `shellpipe` is off?

The whole point of setsid() is to create a new session. That should
not be done by vim. If that's what the user wants, he can do :!setsid
$foo.

> There is a reason this code was
> added. It was done long ago, I don't recall how to reproduce the
> problem.

Unfortunately there is no history for that code, it's there in the
first tag (7.0001).

grepping the code there are some mention of issues with child
processes before they do setsid() themselves. Maybe some tests could
be done adding a delay before executing the child and hitting
(Ctrl-C), or perhaps with a fake shell that never does setsid().

> Just removing the TIOCSCTTY seems to already fix the problem. So how
> about just deleting these lines:
>
> >
> > -# ifdef TIOCSCTTY
> > - // Try to become controlling tty (probably doesn't work,
> > - // unless run by root)
> > - ioctl(pty_slave_fd, TIOCSCTTY, (char *)NULL);
> > -# endif

Yeah, that should do it. But I don't see the need to create a new
session on every fork.

> I wonder what the simplest way is to test the fix.

I did provide examples with posix_spawn and g_spawn_async that both
reproduce the issue.

Cheers.

--
Felipe Contreras

[Bram Moolenaar]

Felipe Contreras wrote:

> > > Processes are not being started correctly with TIOCSCTTY, which does
> > > succeed after setsid().
> > >
> > > None of that stuff is necessary, including the silencing of SIGHUP.
> > >
> > > It's not even executed when shellpipe is off.
> >
> > I suspect removing setsid() will cause trouble if a terminal with a
> > shell is started and that shell exits.
>
> No, I tested that.

On which systems?

--
I wonder, do vegetarians eat fruit bats?

[Felipe Contreras]

On Mon, May 31, 2021 at 6:02 PM Bram Moolenaar <Br...@moolenaar.net> wrote:
> Felipe Contreras wrote:
>
> > > > Processes are not being started correctly with TIOCSCTTY, which does
> > > > succeed after setsid().
> > > >
> > > > None of that stuff is necessary, including the silencing of SIGHUP.
> > > >
> > > > It's not even executed when shellpipe is off.
> > >
> > > I suspect removing setsid() will cause trouble if a terminal with a
> > > shell is started and that shell exits.
> >
> > No, I tested that.
>
> On which systems?

Linux (5.12.8).

--
Felipe Contreras

[Bram Moolenaar]

Felipe Contreras wrote:

> > > > > Processes are not being started correctly with TIOCSCTTY, which does
> > > > > succeed after setsid().
> > > > >
> > > > > None of that stuff is necessary, including the silencing of SIGHUP.
> > > > >
> > > > > It's not even executed when shellpipe is off.
> > > >
> > > > I suspect removing setsid() will cause trouble if a terminal with a
> > > > shell is started and that shell exits.
> > >
> > > No, I tested that.
> >
> > On which systems?
>
> Linux (5.12.8).

There are a dozen different Unix-like systems with slightly different
behavior. I care about all of them. It should be possible to at least
test some BSD versions. But there are also SYSV derivatives that
matter, such as HPUX.

It's difficult to test everything, that is why I prefer to make minimal
changes that actually solve a problem. If we don't know why that
setsid() was there, then removing it probably breaks something.

--
MONK: ... and the Lord spake, saying, "First shalt thou take out the Holy Pin,
then shalt thou count to three, no more, no less. Three shalt be the
number thou shalt count, and the number of the counting shalt be three.
Four shalt thou not count, neither count thou two, excepting that thou
then proceed to three. Five is right out. Once the number three, being
the third number, be reached, then lobbest thou thy Holy Hand Grenade of
Antioch towards thou foe, who being naughty in my sight, shall snuff it.

"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

[Dominique Pellé]

Bram Moolenaar <Br...@moolenaar.net> wrote:

> Felipe Contreras wrote:
>
> > > > > > Processes are not being started correctly with TIOCSCTTY, which does
> > > > > > succeed after setsid().
> > > > > >
> > > > > > None of that stuff is necessary, including the silencing of SIGHUP.
> > > > > >
> > > > > > It's not even executed when shellpipe is off.
> > > > >
> > > > > I suspect removing setsid() will cause trouble if a terminal with a
> > > > > shell is started and that shell exits.
> > > >
> > > > No, I tested that.
> > >
> > > On which systems?
> >
> > Linux (5.12.8).
>
> There are a dozen different Unix-like systems with slightly different
> behavior. I care about all of them. It should be possible to at least
> test some BSD versions. But there are also SYSV derivatives that
> matter, such as HPUX.
>
> It's difficult to test everything, that is why I prefer to make minimal
> changes that actually solve a problem. If we don't know why that
> setsid() was there, then removing it probably breaks something.

If a PR is created in github, at least tests would run
in CI not only on Linux but also macOs, FreeBSD and Windows.
Still, it's only a small subset of all possible platforms, but it may
be enough to see whether the change already breaks something.

Regards
Dominique

[Christian Brabandt]

On Di, 01 Jun 2021, Dominique Pellé wrote:

> If a PR is created in github, at least tests would run
> in CI not only on Linux but also macOs, FreeBSD and Windows.
> Still, it's only a small subset of all possible platforms, but it may
> be enough to see whether the change already breaks something.

It would be good, if that specific test (to run an interactive command
using :!) could be reproduced on the CI systems. Not sure how easy this
would be to do :/

Best
Christian
--
Das Schöpferische wirkt erhabenes Gelingen, fördernd durch Beharrlichkeit.
-- I Ging

[Felipe Contreras]

On Tue, Jun 1, 2021 at 3:12 AM Bram Moolenaar <Br...@moolenaar.net> wrote:
> Felipe Contreras wrote:
>
> > > > > > Processes are not being started correctly with TIOCSCTTY, which does
> > > > > > succeed after setsid().
> > > > > >
> > > > > > None of that stuff is necessary, including the silencing of SIGHUP.
> > > > > >
> > > > > > It's not even executed when shellpipe is off.
> > > > >
> > > > > I suspect removing setsid() will cause trouble if a terminal with a
> > > > > shell is started and that shell exits.
> > > >
> > > > No, I tested that.
> > >
> > > On which systems?
> >
> > Linux (5.12.8).
>
> There are a dozen different Unix-like systems with slightly different
> behavior. I care about all of them. It should be possible to at least
> test some BSD versions. But there are also SYSV derivatives that
> matter, such as HPUX.

Yes, but they all try to follow POSIX, and what setsid is supposed to
do is defined:

https://pubs.opengroup.org/onlinepubs/009604599/functions/setsid.html

setsid isn't going to create a new session on Linux, and kill your cat on HPUX.

It's supposed to be used when you want to start a daemon that is
detached from the parent process, and the terminal.

https://unix.stackexchange.com/questions/240646/why-we-use-setsid-while-daemonizing-a-process

:! should not start a daemon (especially depending on gui_running and shelltmp).

> It's difficult to test everything, that is why I prefer to make minimal
> changes that actually solve a problem.

I agree. For this particular problem the minimal change is enough.

> If we don't know why that
> setsid() was there, then removing it probably breaks something.

But I don't agree with this. It's not a good practice to leave code we
don't know what it's doing, and it has been there for 25 years.

At some point you need to revise code.

I can do an investigation of what's going on and why it might have
been desirable in 1996 to do setsid(). But to blankly state that it
probably breaks something is an assumption. We don't know that. It's
possible it simply was copied from somewhere because somebody else did
it for some reason.

Either way that's a separate topic.

Cheers.

--
Felipe Contreras

[Felipe Contreras]

On Tue, Jun 1, 2021 at 4:19 AM Christian Brabandt <cbl...@256bit.org> wrote:
> On Di, 01 Jun 2021, Dominique Pellé wrote:
>
> > If a PR is created in github, at least tests would run
> > in CI not only on Linux but also macOs, FreeBSD and Windows.
> > Still, it's only a small subset of all possible platforms, but it may
> > be enough to see whether the change already breaks something.
>
> It would be good, if that specific test (to run an interactive command
> using :!) could be reproduced on the CI systems. Not sure how easy this
> would be to do :/

Does the CI run GUI tests?

--
Felipe Contreras

[Bram Moolenaar]

Felipe Contreras wrote:

> > > > > > > Processes are not being started correctly with TIOCSCTTY, which does
> > > > > > > succeed after setsid().
> > > > > > >
> > > > > > > None of that stuff is necessary, including the silencing of SIGHUP.
> > > > > > >
> > > > > > > It's not even executed when shellpipe is off.
> > > > > >
> > > > > > I suspect removing setsid() will cause trouble if a terminal with a
> > > > > > shell is started and that shell exits.
> > > > >
> > > > > No, I tested that.
> > > >
> > > > On which systems?
> > >
> > > Linux (5.12.8).
> >
> > There are a dozen different Unix-like systems with slightly different
> > behavior. I care about all of them. It should be possible to at least
> > test some BSD versions. But there are also SYSV derivatives that
> > matter, such as HPUX.
>
> Yes, but they all try to follow POSIX, and what setsid is supposed to
> do is defined:
>
> https://pubs.opengroup.org/onlinepubs/009604599/functions/setsid.html
>
> setsid isn't going to create a new session on Linux, and kill your cat
> on HPUX.
>
> It's supposed to be used when you want to start a daemon that is
> detached from the parent process, and the terminal.
>
> https://unix.stackexchange.com/questions/240646/why-we-use-setsid-while-daemonizing-a-process
>
> :! should not start a daemon (especially depending on gui_running and
> shelltmp).

It can start a background process:

:!startMyServer&

That's what it is for. Vim is not trying to parse the shell command to
find the "&", since shell syntax is too complicated, thus we use
setsid() always. And if we would not use setsid() then killing Vim
would also kill the process that was started from Vim.

> > It's difficult to test everything, that is why I prefer to make minimal
> > changes that actually solve a problem.
>
> I agree. For this particular problem the minimal change is enough.
>
> > If we don't know why that
> > setsid() was there, then removing it probably breaks something.
>
> But I don't agree with this. It's not a good practice to leave code we
> don't know what it's doing, and it has been there for 25 years.
>
> At some point you need to revise code.
>
> I can do an investigation of what's going on and why it might have
> been desirable in 1996 to do setsid(). But to blankly state that it
> probably breaks something is an assumption. We don't know that. It's
> possible it simply was copied from somewhere because somebody else did
> it for some reason.
>
> Either way that's a separate topic.

Ideally everything is perfectly documented, commented, tested, etc.
The reality is that we often don't have the time, and we just fix a
problem and move on. Otherwise we would have perfectly written code
with only 10% of the features we have now.

--
John: When I'm playing tennis with friends I always get carried away
George: You hurt your foot each time?

[Bram Moolenaar]

Yes, there is one combination in github actions:
linux (huge, gcc, testgui, true)

--
ARTHUR: But if he was dying, he wouldn't bother to carve
"Aaaaarrrrrrggghhh". He'd just say it.
BROTHER MAYNARD: It's down there carved in stone.
GALAHAD: Perhaps he was dictating.

"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

[Felipe Contreras]

This doesn't start a background process:

:!sleep 60

> Vim is not trying to parse the shell command to
> find the "&", since shell syntax is too complicated,

Vim shouldn't parse anything.

If the user wants a background process, he can type the command to
start a background process. If he doesn't, he doesn't.

> thus we use setsid() always.

But we don't use setsid() always.

This doesn't use setsid():

vim -c ':!sleep 60&'

Neither does this:

vim -g -c 'set noshelltemp' -c ':!sleep 60&'

Neither does this:

vim -g -c ':call system("sleep 60&")'

This doesn't work (apparently because of TIOCSCTTY):

vim -g -c 'set shelltemp' -c ':!sleep 60&'

And only *this* sets the setsid:

vim -g -c 'set shelltemp' -c ':!sleep 60'

If setsid was really needed all the commands above would have a
problem, but they don't.

> And if we would not use setsid() then killing Vim
> would also kill the process that was started from Vim.

No it wouldn't. Killing a parent doesn't kill the children.

Try running this program and then kill it.

#include <unistd.h>

int main(void)
{
char *const argv[] = { "/usr/bin/sleep", "60", NULL };
if (fork() == 0)
return execvp(argv[0], argv);
else
return pause();
}

The sleep process will remain there.

> > I can do an investigation of what's going on and why it might have
> > been desirable in 1996 to do setsid(). But to blankly state that it
> > probably breaks something is an assumption. We don't know that. It's
> > possible it simply was copied from somewhere because somebody else did
> > it for some reason.
> >
> > Either way that's a separate topic.
>
> Ideally everything is perfectly documented, commented, tested, etc.
> The reality is that we often don't have the time, and we just fix a
> problem and move on. Otherwise we would have perfectly written code
> with only 10% of the features we have now.

Code that doesn't get cleaned up eventually gets rewritten.

Everything needs to get cleaned up eventually.

Cheers.

--
Felipe Contreras