zsh-workers
 help / color / Atom feed
* Opening a file descriptor for reading with exec breaks vim shell
@ 2020-05-15  4:57 Roman Perepelitsa
  2020-05-17  9:03 ` Stephane Chazelas
  0 siblings, 1 reply; 4+ messages in thread
From: Roman Perepelitsa @ 2020-05-15  4:57 UTC (permalink / raw)
  To: Zsh hackers list

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Opening a file descriptor for reading with exec breaks vim shell
  2020-05-15  4:57 Opening a file descriptor for reading with exec breaks vim shell Roman Perepelitsa
@ 2020-05-17  9:03 ` Stephane Chazelas
  2020-05-18  8:35   ` Roman Perepelitsa
  2020-05-19 17:31   ` Peter Stephenson
  0 siblings, 2 replies; 4+ messages in thread
From: Stephane Chazelas @ 2020-05-17  9:03 UTC (permalink / raw)
  To: Roman Perepelitsa, Zsh hackers list

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;

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Opening a file descriptor for reading with exec breaks vim shell
  2020-05-17  9:03 ` Stephane Chazelas
@ 2020-05-18  8:35   ` Roman Perepelitsa
  2020-05-19 17:31   ` Peter Stephenson
  1 sibling, 0 replies; 4+ messages in thread
From: Roman Perepelitsa @ 2020-05-18  8:35 UTC (permalink / raw)
  To: Zsh hackers list; +Cc: stephane

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Opening a file descriptor for reading with exec breaks vim shell
  2020-05-17  9:03 ` Stephane Chazelas
  2020-05-18  8:35   ` Roman Perepelitsa
@ 2020-05-19 17:31   ` Peter Stephenson
  1 sibling, 0 replies; 4+ messages in thread
From: Peter Stephenson @ 2020-05-19 17:31 UTC (permalink / raw)
  To: Zsh hackers list

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-15  4:57 Opening a file descriptor for reading with exec breaks vim shell Roman Perepelitsa
2020-05-17  9:03 ` Stephane Chazelas
2020-05-18  8:35   ` Roman Perepelitsa
2020-05-19 17:31   ` Peter Stephenson

zsh-workers

Archives are clonable: git clone --mirror http://inbox.vuxu.org/zsh-workers

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.zsh.workers


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git