zpty non-functional?

zpty non-functional?

[Valodim Skywalker]

[-- Attachment #1: Type: text/plain, Size: 308 bytes --]

Hey there,

I wanted to use zpty, but I can't even seem to get a trivial case using
cat to work:

$ zpty x cat
$ echo hello | zpty -w x
$ zpty -r x

I tested on 4.3.11 and 5.0.2 with and without -b, and couldn't get any
sort of data through the cat process. Am I missing something? Or is zpty
borked?

 - V


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: zpty non-functional?

[Bart Schaefer]

On Aug 24,  2:44pm, Valodim Skywalker wrote:
}
} I tested on 4.3.11 and 5.0.2 with and without -b, and couldn't get any
} sort of data through the cat process. Am I missing something? Or is zpty
} borked?

The completion system tests in "make check" rely on zpty, so it's quite
definitely not borked in the general case.  However ...

} $ zpty x cat
} $ echo hello | zpty -w x
} $ zpty -r x

A quick check with "ps"/"strace" indicates that "cat" is being stopped with
a TTIN signal.  That's kind of odd, it means that in spite of being on a
new pty it is not being "attached" to that tty as group leader.  I would say
this is in fact a bug in zpty.

Re: zpty non-functional?

[Peter Stephenson]

On Sat, 24 Aug 2013 13:10:41 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> A quick check with "ps"/"strace" indicates that "cat" is being stopped with
> a TTIN signal.  That's kind of odd, it means that in spite of being on a
> new pty it is not being "attached" to that tty as group leader.  I would say
> this is in fact a bug in zpty.

Hmm... this is well outside my comfort zone.  I thought the following
existing code:

#ifdef TIOCSCTTY
	ioctl(slave, TIOCSCTTY, 0);
#endif

should have that effect (man tty_ioctl, all the following on Linux, actually Fedora 15):

       TIOCSCTTY int arg
              Make  the given terminal the controlling terminal of the calling
              process.

But it apparently doesn't, and sure enough with the patch below...

% zpty x cat
% zpty -w x this is some text
% zpty -r x var
% print $var
this is some text

(I think you need that "var" there, otherwise zpty waits for the process
to exit, which isn't what you want.  This is not the only place the
syntax is a bit wayward...)

The following is after the setsid() (or setpgrp() if that fails or
doesn't exist).  Maybe it should be conditional on one of those
succeeding (and do we *really* want to do setpgrp() if setsid() fails,
even with a warning)?  Or maybe I'm missing the point entirely and just
got lucky?


diff --git a/Src/Modules/zpty.c b/Src/Modules/zpty.c
index 25ec7df..f31e0ba 100644
--- a/Src/Modules/zpty.c
+++ b/Src/Modules/zpty.c
@@ -344,6 +344,7 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock)
 
 	if (get_pty(0, &slave))
 	    exit(1);
+	attachtty(mypid);
 #ifdef TIOCGWINSZ
 	/* Set the window size before associating with the terminal *
 	 * so that we don't get hit with a SIGWINCH.  I'm paranoid. */


-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: zpty non-functional?

[Bart Schaefer]

On Aug 24, 10:48pm, Peter Stephenson wrote:
}
} Hmm... this is well outside my comfort zone.  I thought the following
} existing code:
} 
} #ifdef TIOCSCTTY
} 	ioctl(slave, TIOCSCTTY, 0);
} #endif
} 
}        TIOCSCTTY int arg
}               Make  the given terminal the controlling terminal of the calling
}               process.
} 
} But it apparently doesn't

It makes the terminal the controlling terminal, but it doesn't make the
process the group leader for that terminal.  I suspect we also need the

	ioctl(slave, TIOCSPGRP, mypid);

[or equivalent, e.g. tcsetpgrp()] that attachtty() is doing.  I'm a bit
surprised that calling attachtty() works, because it relies on SHTTY
having been set to the right thing, and I don't see that anything in
zpty.c is making sure that slave and SHTTY refer to the same fd; but
perhaps it always is for some indirect reason ...

Is it safe/reasonable to add

	SHTTY = slave;

right before the attachtty() call in your patch?

} The following is after the setsid() (or setpgrp() if that fails or
} doesn't exist).  Maybe it should be conditional on one of those
} succeeding (and do we *really* want to do setpgrp() if setsid() fails,
} even with a warning)?  Or maybe I'm missing the point entirely and just
} got lucky?

The manual page for setsid says:

    The only error which can happen is EPERM. It is returned when the
    process group ID of any process equals the PID of the calling
    process. Thus, in particular, setsid fails if the calling process is
    already a process group leader.

A check of the setpgrp() manual page indicates pretty much the same thing;
there is no defined error that can result from setprp(0L, getpid()); the
only errors are from attempting to set the process group of a different
process than the caller.

So only some completely unexpected error (interrupted system call?) could
cause setsid() to fail, in which case trying again with setpgrp() is no
worse off.

Re: zpty non-functional?

[Peter Stephenson]

On Sat, 24 Aug 2013 17:01:57 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> I'm a bit
> surprised that calling attachtty() works, because it relies on SHTTY
> having been set to the right thing, and I don't see that anything in
> zpty.c is making sure that slave and SHTTY refer to the same fd; but
> perhaps it always is for some indirect reason ...
> 
> Is it safe/reasonable to add
> 
> 	SHTTY = slave;
> 
> right before the attachtty() call in your patch?

That seems to work, anyway.

I added a test, but if instead of using the pipe I add a string as
arguments to zpty -w it usually fails:  I get some of the string
followed by the whole of the string in the output.  This suggests the
write is doing something weird.

I added the trick to make it use _exit() (as we haven't forked) at the
end of the slave code, but that didn't help --- which it probably
shouldn't anyway for various reasons, but I left it in since it looked
logically correct anyway.

So there's something else to investigate.

diff --git a/Src/Modules/zpty.c b/Src/Modules/zpty.c
index 25ec7df..3821194 100644
--- a/Src/Modules/zpty.c
+++ b/Src/Modules/zpty.c
@@ -344,6 +344,8 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock)
 
 	if (get_pty(0, &slave))
 	    exit(1);
+	SHTTY = slave;
+	attachtty(mypid);
 #ifdef TIOCGWINSZ
 	/* Set the window size before associating with the terminal *
 	 * so that we don't get hit with a SIGWINCH.  I'm paranoid. */
@@ -398,6 +400,7 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock)
 	opts[INTERACTIVE] = 0;
 	execode(prog, 1, 0, "zpty");
 	stopmsg = 2;
+	mypid = 0; /* trick to ensure we _exit() */
 	zexit(lastval, 0);
     }
     master = movefd(master);
diff --git a/Test/.distfiles b/Test/.distfiles
index 689b695..ab92153 100644
--- a/Test/.distfiles
+++ b/Test/.distfiles
@@ -40,6 +40,7 @@ V04features.ztst
 V05styles.ztst
 V06parameter.ztst
 V07pcre.ztst
+V08zpty.ztst
 Y01completion.ztst
 Y02compmatch.ztst
 Y03arguments.ztst
diff --git a/Test/V08zpty.ztst b/Test/V08zpty.ztst
new file mode 100644
index 0000000..d9d24c5
--- /dev/null
+++ b/Test/V08zpty.ztst
@@ -0,0 +1,20 @@
+# zpty is required by tests of interactive modes of the shell itself.
+# This tests some extra things.
+
+%prep
+
+  if ! zmodload zsh/zpty 2>/dev/null
+  then
+    ZTST_unimplemented="the zsh/zpty module is not available"
+    return 0
+  fi
+
+%test
+
+  zpty cat cat
+  print a line of text | zpty -w cat
+  var=
+  zpty -r cat var && print -r -- ${var%%$'\r\n'}
+  zpty -d cat
+0:zpty with a process that does not set up the terminal
+>a line of text

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: zpty non-functional?

[Bart Schaefer]

On Aug 25,  7:59pm, Peter Stephenson wrote:
}
} I added a test, but if instead of using the pipe I add a string as
} arguments to zpty -w it usually fails:  I get some of the string
} followed by the whole of the string in the output.  This suggests the
} write is doing something weird.

I modified the test to use "zpty cat 'strace cat 2>file'" and to use
"zpty -w cat sample strings" instead of the pipe, then examined the
strace output.  The "cat" process always reads/writes exactly what it
is meant to, even when the test fails.

I then suspected that the problem is with "ztpy -r" not with -w.  If
I change the test to write two lines:

	print a line of text | zpty -w cat
	var=; zpty -r cat var
	zpty -w cat sample strings
	var=; zpty -r cat var

then it always succeeds.  If I swap the order so that the pipe is the
second of the two writes, then it always *fails*, repeating the same
output ("sample strings") twice, but strace still shows cat reading
and writing the correct two lines in the correct order.

Further testing by using "cat > /dev/null" shows that when "zpty -w"
is used, the pty acts as if it is in echo mode, so "zpty -r" reads
back what zpty -w wrote.  In the pipe mode, for some reason, this does
not happen.  ptywrite() is called just once in either case, and I
confirmed that ptysettyinfo() is being called at pty setup.

If I put in some debugging statements such as zwarn calls in ptywrite,
then I sometimes get only partial echo behavior, e.g., I'll get back
"samplesample strings" and then "a line of text".

I also tried adding a pid > 0 branch to the zfork conditional in which
close(slave) is done [which seems like the right thing anyway] but that
did not fix the behavior, although I thought briefly that it changed it.

Finally I tried putting a "sleep 1" in the test before the first write
to the pty.  This makes it succeed 100% of the time, regardless of the
order of the pipe or direct write.

This leads me to conclude that it's a race condition:  The master writer
may read back what it wrote, up until the point that there is a process
on the slave side consuming it instead.

Whether this means we want to copy the synch[] pipe hack from exec.c to
prevent zpty from returning before the child process is running, or just
document the possible race, I will leave up to consensus (or PWS).

Re: zpty non-functional?

[Peter Stephenson]

On Sun, 25 Aug 2013 15:01:04 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> This leads me to conclude that it's a race condition:  The master writer
> may read back what it wrote, up until the point that there is a process
> on the slave side consuming it instead.

That all sounds entirely convincing, thanks for the work.
 
> Whether this means we want to copy the synch[] pipe hack from exec.c to
> prevent zpty from returning before the child process is running, or just
> document the possible race, I will leave up to consensus (or PWS).

Here's a synch, as well as a test that was showing up the problem
(though not every time).  I've tried to catch as many funny cases for
read/write as I could think of; this may be overkill but should be at
worst harmless (unless there's still something I've missed).

diff --git a/Src/Modules/zpty.c b/Src/Modules/zpty.c
index 3821194..fca0cc1 100644
--- a/Src/Modules/zpty.c
+++ b/Src/Modules/zpty.c
@@ -293,8 +293,8 @@ static int
 newptycmd(char *nam, char *pname, char **args, int echo, int nblock)
 {
     Ptycmd p;
-    int master, slave, pid, oineval = ineval;
-    char *oscriptname = scriptname;
+    int master, slave, pid, oineval = ineval, ret;
+    char *oscriptname = scriptname, syncch;
     Eprog prog;
 
     /* code borrowed from bin_eval() */
@@ -398,6 +398,20 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock)
 	setsparam("TTY", ztrdup(ttystrname));
 
 	opts[INTERACTIVE] = 0;
+
+	syncch = 0;
+	do {
+	    ret = write(1, &syncch, 1);
+	} while (ret != 1 && (
+#ifdef EWOULDBLOCK
+	    errno == EWOULDBLOCK ||
+#else
+#ifdef EAGAIN
+	    errno == EAGAIN ||
+#endif
+#endif
+	    errno == EINTR));
+
 	execode(prog, 1, 0, "zpty");
 	stopmsg = 2;
 	mypid = 0; /* trick to ensure we _exit() */
@@ -433,6 +447,18 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock)
     scriptname = oscriptname;
     ineval = oineval;
 
+    do {
+	ret = read(master, &syncch, 1);
+    } while (ret != 1 && (
+#ifdef EWOULDBLOCK
+	    errno == EWOULDBLOCK ||
+#else
+#ifdef EAGAIN
+	    errno == EAGAIN ||
+#endif
+#endif
+	    errno == EINTR));
+
     return 0;
 }
 
diff --git a/Test/V08zpty.ztst b/Test/V08zpty.ztst
index d9d24c5..5b08fc2 100644
--- a/Test/V08zpty.ztst
+++ b/Test/V08zpty.ztst
@@ -12,9 +12,17 @@
 %test
 
   zpty cat cat
+  zpty -w cat a line of text
+  var=
+  zpty -r cat var && print -r -- ${var%%$'\r\n'}
+  zpty -d cat
+0:zpty with a process that does not set up the terminal: internal write
+>a line of text
+
+  zpty cat cat
   print a line of text | zpty -w cat
   var=
   zpty -r cat var && print -r -- ${var%%$'\r\n'}
   zpty -d cat
-0:zpty with a process that does not set up the terminal
+0:zpty with a process that does not set up the terminal: write via stdin
 >a line of text

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: zpty non-functional?

[Bart Schaefer]

On Aug 26,  8:33pm, Peter Stephenson wrote:
}
} Here's a synch, as well as a test that was showing up the problem

It just occurred to me that if the syncch [why two c?] trick solves
the race, then it's probably not even necessary to write a byte on
the pipe -- just close it in the child, and the parent will get EOF.

On the other hand it also occurred to me that this might not always
solve the race.  It'd be better if syncch could be close-on-exec ...
except that execode() might not actually do an exec() ...

So this is probably the best we can do.

-- 
Barton E. Schaefer

Re: zpty non-functional?

[Peter Stephenson]

On Mon, 26 Aug 2013 17:54:21 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Aug 26,  8:33pm, Peter Stephenson wrote:
> }
> } Here's a synch, as well as a test that was showing up the problem
> 
> It just occurred to me that if the syncch [why two c?] trick solves
> the race, then it's probably not even necessary to write a byte on
> the pipe -- just close it in the child, and the parent will get EOF.

syncch is "sync ch[ar]".

All I've added is the read and write --- the pipe was there anyway as
the key part of the master/slave interface.  So this is about as minimal
as we get, up to tests and loops I may have added that we don't really
need.

pws

Re: zpty non-functional?

[Bart Schaefer]

On Aug 27,  1:01pm, Peter Stephenson wrote:
}
} 
} All I've added is the read and write --- the pipe was there anyway as
} the key part of the master/slave interface.

Ah, sorry; I misread the patch.  In exec.c synch is the array that holds
the pair of pipe descriptors, so I was expecting a new pipe here.

I spent a while wondering whether it could ever matter to either side of
the pty that it had been used to transmit an extra byte before the real
conversation started, but I didn't come up with any objection.