Re: Regression: 'read -s' does not disable echo

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

> On 02/01/2024 19:32 GMT Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
> 
>  
> On Tue, Jan 2, 2024 at 9:05 AM Peter Stephenson
> <p.w.stephenson@xxxxxxxxxxxx> wrote:
> >
> > > On 02/01/2024 15:25 GMT Phil Pennock <zsh-workers+phil.pennock@xxxxxxxxxxxx> wrote:
> > >
> > > Folks, there's a regression on git mainline such that `read -s` does not
> > > suppress terminal echo.
> >
> > That's not very friendly.  I guess it's to do with the way
> > readfd is handled.  Something like this, or more general?
> 
> This dates from workers/51969 (commit aa85564), which means this may
> have affected the -d option as well.  At the time I said "would
> appreciate another eyeball" but never got one ...

Looks like -s and -d are parallel, yes.

> > -    if (OPT_ISSET(ops,'s') && SHTTY == readfd) {
> > +    if (OPT_ISSET(ops,'s') &&
> > +       (SHTTY == readfd || (readfd == 0 && isatty(0)))) {
> 
> Previous tests here were for (SHTTY != -1) which was causing incorrect
> calls to gettyinfo() when shell input was not closed but stdin was
> redirected.  I'm now unsure whether (SHTTY == readfd) is ever true
> here?  I expect the original code was trying to avoid the overhead of
> isatty().

It looks like there are a few cases that deal with key input,
so they are relevant.  I tried read -ks and that does work.

> However, there may be a deeper problem:  gettyinfo() always reads the
> state of the controlling TTY, but it's possible that "read -s" has
> been called with input and output redirected from a different tty
> device.

I suggest we just document "don't do that".  That's definitely not
something I think we ought to be playing with.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index 9e08a1dbc..441a71c45 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -6506,7 +6506,8 @@ bin_read(char *name, char **args, Options ops, UNUSED(int func))
     } else
 	readfd = izle = 0;
 
-    if (OPT_ISSET(ops,'s') && SHTTY == readfd) {
+    if (OPT_ISSET(ops,'s') &&
+	(SHTTY == readfd || (readfd == 0 && isatty(0)))) {
 	struct ttyinfo ti;
 	memset(&ti, 0, sizeof(struct ttyinfo));
 	gettyinfo(&ti);
@@ -6555,7 +6556,7 @@ bin_read(char *name, char **args, Options ops, UNUSED(int func))
         delim = (unsigned char) ((delimstr[0] == Meta) ?
 			delimstr[1] ^ 32 : delimstr[0]);
 #endif
-	if (SHTTY == readfd) {
+	if (SHTTY == readfd || (readfd == 0 && isatty(0))) {
 	    struct ttyinfo ti;
 	    gettyinfo(&ti);
 	    if (! resettty) {