Regression: 'read -s' does not disable echo

Regression: 'read -s' does not disable echo

[Phil Pennock]

Folks, there's a regression on git mainline such that `read -s` does not
suppress terminal echo.

I recompiled yesterday, for the first time since ... I think last March.
I don't have the complete patch-level I was at before, sorry.  I'm now
at commit `a528af5c5` (or one after, for a local patch).

Running:

    read -s -r 'SUPER_SECRET?Enter your super secret: '

should read into $SUPER_SECRET without terminal echo.  The '-r' is not
needed to reproduce, but included to show that yes, I'm considering
that.

Reproduces when started with `zsh -f`.

~% zsh -f
fullerene% read -s -r 'SUPER_SECRET?Enter your super secret: '
Enter your super secret: wobble
fullerene% echo $ZSH_VERSION $ZSH_PATCHLEVEL
5.9.0.1-dev pdp/phil-local-patches/zsh-5.9-341-gb6bd6cc0c

Environment is Linux (Ubuntu 20.04, amd64/x86_64) in gnome-terminal,
reproduces in xterm too.

-Phil

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

[Peter Stephenson]

> On 02/01/2024 15:25 GMT Phil Pennock <zsh-workers+phil.pennock@spodhuis.org> 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?
Simpler is probably better here.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index 9e08a1dbc..dc651f687 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);

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

[Bart Schaefer]

On Tue, Jan 2, 2024 at 9:05 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> > On 02/01/2024 15:25 GMT Phil Pennock <zsh-workers+phil.pennock@spodhuis.org> 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 ...

> -    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().

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.

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

[Phil Pennock]

On 2024-01-02 at 17:05 +0000, Peter Stephenson wrote:
> That's not very friendly.  I guess it's to do with the way
> readfd is handled.  Something like this, or more general?
> Simpler is probably better here.

I can't comment on Bart's observations about the correctness of this
fix, but as a fast fix for my particular problem right now, I can
confirm that this seems to work for me.

Thank you, and sorry for throwing out the problem report without a patch
or deeper investigation this morning.

-Phil

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

[Peter Stephenson]

> On 02/01/2024 19:32 GMT Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
>  
> On Tue, Jan 2, 2024 at 9:05 AM Peter Stephenson
> <p.w.stephenson@ntlworld.com> wrote:
> >
> > > On 02/01/2024 15:25 GMT Phil Pennock <zsh-workers+phil.pennock@spodhuis.org> 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) {

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

[Bart Schaefer]

On Wed, Jan 3, 2024 at 2:03 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> > On 02/01/2024 19:32 GMT Bart Schaefer <schaefer@brasslantern.com> wrote:
> >
> > 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.

Well, that's just it.  With the isatty(0) test, we can be duped into
changing the state of the controlling TTY by directing input from a
different one.

So either "read -s" should ALWAYS read from SHTTY, or something more
involved is needed here.

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

[Peter Stephenson]

> On 03/01/2024 18:18 GMT Bart Schaefer <schaefer@brasslantern.com> wrote:
> So either "read -s" should ALWAYS read from SHTTY, or something more
> involved is needed here.

We could use ttyname to check that SHTTY and 0 are the same device.
Then we'd just document that -s and -d (which needs a further patch)
only work on the shell's own terminal.  The following seems to
do what I expect on Ubuntu.

The only simpler alternative I see is just ignoring fd 0 completely,
but that doesn't sound too safe.  I don't think terminal operations
need to be particularly efficient so unless there are OS oddities
with ttyname() this might be good enough.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index 9e08a1dbc..cf24f89c7 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -6506,8 +6506,16 @@ 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;
+	char *tns = dupstring(ttyname(SHTTY));
+	char *tn0 = dupstring(ttyname(0));
+	if (!tns || !tn0 || strcmp(tns, tn0))
+	{
+	    zwarnnam(name, "Bad terminal for read -s: must be shell terminal");
+	    return 1;
+	}
 	memset(&ti, 0, sizeof(struct ttyinfo));
 	gettyinfo(&ti);
 	saveti = ti;
@@ -6555,7 +6563,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) {

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

[Peter Stephenson]

> On 04/01/2024 09:32 GMT Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> 
>  
> > On 03/01/2024 18:18 GMT Bart Schaefer <schaefer@brasslantern.com> wrote:
> > So either "read -s" should ALWAYS read from SHTTY, or something more
> > involved is needed here.
> 
> We could use ttyname to check that SHTTY and 0 are the same device.
> Then we'd just document that -s and -d (which needs a further patch)
> only work on the shell's own terminal.  The following seems to
> do what I expect on Ubuntu.

Arguably you don't need this check if you find readfd == SHTTY,
rather than 0, but I'm not sure how much more complicated I'd
want to make this.

pws

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

[Bart Schaefer]

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

On Thu, Jan 4, 2024 at 1:32 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> We could use ttyname to check that SHTTY and 0 are the same device.
> Then we'd just document that -s and -d (which needs a further patch)
> only work on the shell's own terminal.

That's not compatible with "read -s" on e.g. bash, if that matters.

SHTTY is just a file descriptor number, so this isn't terribly
difficult to work around.

Bit of a hack here: to avoid changing a large number of lines in
utils.c, I hid the global SHTTY behind a local.

I think this actually fixes another potential bug where the global tty
state (in shttyinfo) was restored even though a possibly different
state was in the saveti local.

[-- Attachment #2: read-s.txt --]
[-- Type: text/plain, Size: 3577 bytes --]

diff --git a/Src/builtin.c b/Src/builtin.c
index 9e08a1dbc..5c5adb9d3 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -6506,10 +6506,10 @@ 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') && isatty(readfd)) {
 	struct ttyinfo ti;
 	memset(&ti, 0, sizeof(struct ttyinfo));
-	gettyinfo(&ti);
+	fdgettyinfo(readfd, &ti);
 	saveti = ti;
 	resettty = 1;
 #ifdef HAS_TIO
@@ -6517,7 +6517,7 @@ bin_read(char *name, char **args, Options ops, UNUSED(int func))
 #else
 	ti.sgttyb.sg_flags &= ~ECHO;
 #endif
-	settyinfo(&ti);
+	fdsettyinfo(readfd, &ti);
     }
 
     /* handle prompt */
@@ -6555,9 +6555,9 @@ 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 (isatty(readfd)) {
 	    struct ttyinfo ti;
-	    gettyinfo(&ti);
+	    fdgettyinfo(readfd, &ti);
 	    if (! resettty) {
 	      saveti = ti;
 	      resettty = 1;
@@ -6569,7 +6569,7 @@ bin_read(char *name, char **args, Options ops, UNUSED(int func))
 #else
 	    ti.sgttyb.sg_flags |= CBREAK;
 #endif
-	    settyinfo(&ti);
+	    fdsettyinfo(readfd, &ti);
 	}
     }
     if (OPT_ISSET(ops,'t')) {
@@ -6604,8 +6604,8 @@ bin_read(char *name, char **args, Options ops, UNUSED(int func))
 			   timeout)) {
 		if (keys && !zleactive && !isem)
 		    settyinfo(&shttyinfo);
-		else if (resettty && SHTTY != -1)
-		    settyinfo(&saveti);
+		else if (resettty)
+		    fdsettyinfo(readfd, &saveti);
 		if (haso) {
 		    if (shout)
 			fclose(shout);
@@ -6717,7 +6717,7 @@ bin_read(char *name, char **args, Options ops, UNUSED(int func))
 	    if (isem)
 		while (val > 0 && read(SHTTY, &d, 1) == 1 && d != '\n');
 	    else if (resettty) {
-		settyinfo(&shttyinfo);
+		fdsettyinfo(readfd, &saveti);
 		resettty = 0;
 	    }
 	    if (haso) {
@@ -6746,8 +6746,8 @@ bin_read(char *name, char **args, Options ops, UNUSED(int func))
 	    setsparam(reply, metafy(buf, bptr - buf, META_REALLOC));
 	else
 	    zfree(buf, bptr - buf + 1);
-	if (resettty && SHTTY != -1)
-	    settyinfo(&saveti);
+	if (resettty)
+	    fdsettyinfo(readfd, &saveti);
 	return eof;
     }
 
@@ -6957,8 +6957,8 @@ bin_read(char *name, char **args, Options ops, UNUSED(int func))
 	    *pp++ = NULL;
 	    setaparam(reply, p);
 	}
-	if (resettty && SHTTY != -1)
-	    settyinfo(&saveti);
+	if (resettty)
+	    fdsettyinfo(readfd, &saveti);
 	return c == EOF;
     }
     buf = bptr = (char *)zalloc(bsiz = 64);
@@ -7086,8 +7086,8 @@ bin_read(char *name, char **args, Options ops, UNUSED(int func))
 	    break;
     }
     *bptr = '\0';
-    if (resettty && SHTTY != -1)
-	settyinfo(&saveti);
+    if (resettty)
+	fdsettyinfo(readfd, &saveti);
     /* final assignment of reply, etc. */
     if (OPT_ISSET(ops,'e') || OPT_ISSET(ops,'E')) {
 	zputs(buf, stdout);
diff --git a/Src/utils.c b/Src/utils.c
index 0f66984cd..1a4f4c14b 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -1730,6 +1730,13 @@ freestr(void *a)
 /**/
 mod_export void
 gettyinfo(struct ttyinfo *ti)
+{
+    fdgettyinfo(SHTTY, ti);
+}
+
+/**/
+mod_export void
+fdgettyinfo(int SHTTY, struct ttyinfo *ti)
 {
     if (SHTTY != -1) {
 #ifdef HAVE_TERMIOS_H
@@ -1755,6 +1762,13 @@ gettyinfo(struct ttyinfo *ti)
 /**/
 mod_export void
 settyinfo(struct ttyinfo *ti)
+{
+    fdsettyinfo(SHTTY, ti);
+}
+
+/**/
+mod_export void
+fdsettyinfo(int SHTTY, struct ttyinfo *ti)
 {
     if (SHTTY != -1) {
 #ifdef HAVE_TERMIOS_H