Opening a file descriptor for reading with exec breaks vim shell

Opening a file descriptor for reading with exec breaks vim shell

[Roman Perepelitsa]

Prerequisites:

1. Install vim.
2. Configure vim so that :shell command opens zsh. Setting zsh as your
login shell is one way to do it.

Reproduce:

  adam% vim
  :shell
  adam% exec {fd}</dev/null
  adam% exit
  zsh: suspended (tty output)  vim
  adam%

The expected behavior is to not have vim suspended.

Replacing `exec {fd}</dev/null` with any of the following fixes the problem:

  exec 3</dev/null
  exec {fd}>/dev/null
  zmodload zsh/system && sysopen -ru fd /dev/null
  zmodload zsh/system && sysopen -ru fd1 /dev/null && exec {fd2}<&$fd1

Replacing `exec {fd}</dev/null` with any of the following does NOT fix
the problem:

  exec 0</dev/null
  exec {fd}</dev/null && exec {fd}<&-

All tests were performed with zsh-5.8-131-g06c0a39 (tip of master as
of this writing) and no rc files.

Roman.

Re: Opening a file descriptor for reading with exec breaks vim shell

[Stephane Chazelas]

2020-05-15 06:57:10 +0200, Roman Perepelitsa:
[...]
> Reproduce:
> 
>   adam% vim
>   :shell
>   adam% exec {fd}</dev/null
>   adam% exit
>   zsh: suspended (tty output)  vim
[...]

Can be reproduced with:

~$ sh -c 'zsh -f; read x'
% exec {fd}</dev/null
% exit
zsh: suspended (tty input)  sh -c 'zsh -f; read x'

Also happens if you run set +m -m in that zsh.

What happens is that here zsh is not a group leader on startup
so creates a new process group and makes it the forground job.

It normally restores the foreground group upon exit, but when
running those exec or set -m commands, that doesn't happen.

That is because in those cases, the origpgrp variable (which
records the original process group to be restored on exit) is
being incorrectly overridden. In the case of "exec {fd}<",
because init_io is being called by mistake.

Note that in any case, and with any shell with job control
support, you get the same problem if you run kill -s KILL "$$"
in that shell.

The patch below fixes those, but I have to admit there's a lot
of the related code in zsh I don't understand, a lot of the
behaviour I don't understand either. It's possible that patch
could break things related to clone, zpty, set -m or opening tty
devices, but it seems to me that some of that is already quite
broken.

For instance, set -m doesn't seem to work (or at least do
anything useful). In  non-interactive shells, it  does seem to
cause jobs to be put in their own process groups, but not them
being put in foreground/background of the controlling terminal
so it's not really "job control" (as the doc says).

In interactive shells started with zsh +m, set -m doesn't seem
to be usable (causes lock ups probably caused by some SIGTTIN),
regardless of whether zsh is started as session, group leader or
not.

I also tried starting an interactive zsh +m with no controlling
terminal (but still with I/O to one so I could enter commands)
as a session leader, then do a exec <> some-tty >&0 2>&0 (where
some-tty had no controlling session). That did not attach that
terminal to zsh's session (I suppose because <> calls open()
with O_NOCTTY, same with sysopen), and set -m had no effect
(monitor option remained off).

diff --git a/Src/exec.c b/Src/exec.c
index 2b8e2167f..5bd317880 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -3669,7 +3669,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		addfd(forked, save, mfds, fn->fd1, fil, 0, fn->varid);
 		/* If this is 'exec < file', read from stdin, *
 		 * not terminal, unless `file' is a terminal. */
-		if (nullexec == 1 && fn->fd1 == 0 &&
+		if (nullexec == 1 && fn->fd1 == 0 && !fn->varid &&
 		    isset(SHINSTDIN) && interact && !zleactive)
 		    init_io(NULL);
 		break;
diff --git a/Src/init.c b/Src/init.c
index 3d6c94d04..342f0a697 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -697,11 +697,14 @@ init_io(char *cmd)
      * process group leader.
      */
     mypid = (zlong)getpid();
-    if (opts[MONITOR] && (SHTTY != -1)) {
-	origpgrp = GETPGRP();
-        acquire_pgrp(); /* might also clear opts[MONITOR] */
-    } else
-	opts[MONITOR] = 0;
+    if (opts[MONITOR]) {
+	if (SHTTY == -1)
+	    opts[MONITOR] = 0;
+	else if (!origpgrp) {
+	    origpgrp = GETPGRP();
+	    acquire_pgrp(); /* might also clear opts[MONITOR] */
+	}
+    }
 #else
     opts[MONITOR] = 0;
 #endif
diff --git a/Src/options.c b/Src/options.c
index 7586d21d2..cf3664218 100644
--- a/Src/options.c
+++ b/Src/options.c
@@ -878,11 +878,12 @@ dosetopt(int optno, int value, int force, char *new_opts)
     } else if (!force && optno == MONITOR && value) {
 	if (new_opts[optno] == value)
 	    return 0;
-	if (SHTTY != -1) {
+	if (SHTTY == -1)
+	    return -1;
+	if (!origpgrp) {
 	    origpgrp = GETPGRP();
 	    acquire_pgrp();
-	} else
-	    return -1;
+	}
 #else
     } else if(optno == MONITOR && value) {
 	    return -1;

Re: Opening a file descriptor for reading with exec breaks vim shell

[Roman Perepelitsa]

On Sun, May 17, 2020 at 11:03 AM Stephane Chazelas
<stephane@chazelas.org> wrote:
>
> The patch below fixes those [...]

I confirm that it does.

Thanks, Stephane.

Roman.

Re: Opening a file descriptor for reading with exec breaks vim shell

[Peter Stephenson]

On 17 May 2020 at 10:03 Stephane Chazelas <stephane@chazelas.org> wrote:
> The patch below fixes those, but I have to admit there's a lot
> of the related code in zsh I don't understand, a lot of the
> behaviour I don't understand either. It's possible that patch
> could break things related to clone, zpty, set -m or opening tty
> devices, but it seems to me that some of that is already quite
> broken.

Should really follow up to this...

Actually, I don't think set -m was ever mended; I think it's always
been a bit of an afterthought that sets up a process group, but
doesn't do a lot else.  I'd be inclined not to worry about
side effects of this patch unless there's something demonstrably
worse, since it's fixing something you would expect to work
(and that doesn't depend on zsh wizardry).

pws