Regression with stdin handling in non-interactive mode between 5.8 and 5.8.1

Regression with stdin handling in non-interactive mode between 5.8 and 5.8.1

[Lyude Paul]

Hi! I'm reporting this here because after some discussion in #zsh, it was
determined this is likely both a regression, and also isn't POSIX compliant.
Keep in mind I don't have as clear of an understanding of what's happening
below the hood here as I'd like, so I'm not 100% the subject line here is
correct. I've got plenty of examples to clarify though :)

Basically, what seems to be happening is that since 5.8.1 zsh no longer seems
to correctly handle input from stdin unless the terminal is in interactive
mode.

Here's a simple example script that demonstrates what I mean:

   printf '%s\n' 'echo Shell is $$' sh 'echo Shell is $$' | zsh

Running on zsh 5.8 returns:

   Shell is 70
   Shell is 71

Running on zsh 5.8.1 however, returns:

   Shell is 86396
   Shell is 86396

This can end up being an issue when trying to do things like (assuming sudo is
configured on this system to not request a password, and zsh is the default
shell):

   ssh foo <<- _EOF_
   whoami
   sudo -s
   whoami
   _EOF_

While that's maybe not the best way of doing such things in shell, I have
quite a number of scripts that rely on this working and have for quite some
time. According to llua from #zsh as well, this is also likely not POSIX
compliant according to:

   https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sh.html (see the
   section labeled "INPUT FILES")

llua suggested that this breakage may have come from
e5cd2dd980302f328d232d933f646c3dc02828bf ("49290: Replace stdio for buffered
shell input."), which I've confirmed to be true by bisecting this locally. 

For reference: I originally reproduced this on Fedora 35, although I have a
feeling that probably doesn't matter too much here. If there's any other
information I can provide that would help with getting this fixed, don't
hesistate to ask. And thank you ahead of time!
-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: Regression with stdin handling in non-interactive mode between 5.8 and 5.8.1

[Bart Schaefer]

On Wed, Mar 2, 2022 at 2:39 PM Lyude Paul <lyude@redhat.com> wrote:
>
>
>    printf '%s\n' 'echo Shell is $$' sh 'echo Shell is $$' | zsh
>
> Running on zsh 5.8.1 however, returns:
>
>    Shell is 86396
>    Shell is 86396

This has to be workers/49290, the replacement I/O buffering is not
doing line-buffered input for non-interactive shells.  An strace shows

read(0, "echo Shell is $$\nsh\necho Shell i"..., 8192) = 37

Re: Regression with stdin handling in non-interactive mode between 5.8 and 5.8.1

[Peter Stephenson]

> On 02 March 2022 at 22:38 Lyude Paul <lyude@redhat.com> wrote:
> 
> 
> Hi! I'm reporting this here because after some discussion in #zsh, it was
> determined this is likely both a regression, and also isn't POSIX compliant.
> Keep in mind I don't have as clear of an understanding of what's happening
> below the hood here as I'd like, so I'm not 100% the subject line here is
> correct. I've got plenty of examples to clarify though :)
> 
> Basically, what seems to be happening is that since 5.8.1 zsh no longer seems
> to correctly handle input from stdin unless the terminal is in interactive
> mode.
> 
> Here's a simple example script that demonstrates what I mean:
> 
>    printf '%s\n' 'echo Shell is $$' sh 'echo Shell is $$' | zsh
> 
> Running on zsh 5.8 returns:
> 
>    Shell is 70
>    Shell is 71
> 
> Running on zsh 5.8.1 however, returns:
> 
>    Shell is 86396
>    Shell is 86396

Thanks for the nice simple test --- I'll try and concoct something similar
but predictable for the shell tests.

I hope it's as simple as the following.  I can't think of any optimisation
or a case where line buffering would be wrong.

pws

diff --git a/Src/input.c b/Src/input.c
index caeaff0e3..50cd2cd78 100644
--- a/Src/input.c
+++ b/Src/input.c
@@ -223,13 +223,20 @@ shingetchar(void)
 	return STOUC(*shinbufptr++);
 
     shinbufreset();
-    do {
+    for (;;) {
 	errno = 0;
-	nread = read(SHIN, shinbuffer, SHINBUFSIZE);
-    } while (nread < 0 && errno == EINTR);
-    if (nread <= 0)
+	nread = read(SHIN, shinbufendptr, 1);
+	if (nread > 0) {
+	    /* Use line buffering (POSIX requirement) */
+	    if (*shinbufendptr++ == '\n')
+		break;
+	    if (shinbufendptr == shinbuffer + SHINBUFSIZE)
+		break;
+	} else if (nread == 0 || errno != EINTR)
+	    break;
+    }
+    if (shinbufendptr == shinbuffer)
 	return -1;
-    shinbufendptr = shinbuffer + nread;
     return STOUC(*shinbufptr++);
 }

Re: Regression with stdin handling in non-interactive mode between 5.8 and 5.8.1

[Peter Stephenson]

> On 03 March 2022 at 09:39 Peter Stephenson <p.w.stephenson@ntlworld.com> wrote> > On 02 March 2022 at 22:38 Lyude Paul <lyude@redhat.com> wrote:
> > Running on zsh 5.8 returns:
> > 
> >    Shell is 70
> >    Shell is 71
> > 
> > Running on zsh 5.8.1 however, returns:
> > 
> >    Shell is 86396
> >    Shell is 86396
> 
> Thanks for the nice simple test --- I'll try and concoct something similar
> but predictable for the shell tests.

Added to the patch; I've confirmed this succeeds or fails as expected.

pws

diff --git a/Src/input.c b/Src/input.c
index caeaff0e3..50cd2cd78 100644
--- a/Src/input.c
+++ b/Src/input.c
@@ -223,13 +223,20 @@ shingetchar(void)
 	return STOUC(*shinbufptr++);
 
     shinbufreset();
-    do {
+    for (;;) {
 	errno = 0;
-	nread = read(SHIN, shinbuffer, SHINBUFSIZE);
-    } while (nread < 0 && errno == EINTR);
-    if (nread <= 0)
+	nread = read(SHIN, shinbufendptr, 1);
+	if (nread > 0) {
+	    /* Use line buffering (POSIX requirement) */
+	    if (*shinbufendptr++ == '\n')
+		break;
+	    if (shinbufendptr == shinbuffer + SHINBUFSIZE)
+		break;
+	} else if (nread == 0 || errno != EINTR)
+	    break;
+    }
+    if (shinbufendptr == shinbuffer)
 	return -1;
-    shinbufendptr = shinbuffer + nread;
     return STOUC(*shinbufptr++);
 }
 
diff --git a/Test/A01grammar.ztst b/Test/A01grammar.ztst
index 4e39a8f3c..0312fe94e 100644
--- a/Test/A01grammar.ztst
+++ b/Test/A01grammar.ztst
@@ -961,3 +961,12 @@ F:Note that the behaviour of 'exit' inside try-list inside a function is unspeci
 F:This test was written to ensure the behaviour doesn't change silently.
 F:If this test fails during development, it *might* be appropriate to change
 F:its expectations.
+
+ (
+   export VALUE=first
+   print -l 'echo Value is $VALUE' 'VALUE=second sh' 'echo Value is $VALUE' |
+   $ZTST_testdir/../Src/zsh -f
+ )
+0:Non-interactive shell command input is line buffered
+>Value is first
+>Value is second

Re: Regression with stdin handling in non-interactive mode between 5.8 and 5.8.1

[Lyude Paul]

Woo - seems to fix the problem on my end! :)

Don't know if y'all use these, but feel free to consider this patch:

Tested-by: Lyude Paul <lyude@redhat.com>

On Thu, 2022-03-03 at 11:52 +0000, Peter Stephenson wrote:
> diff --git a/Src/input.c b/Src/input.c
> index caeaff0e3..50cd2cd78 100644
> --- a/Src/input.c
> +++ b/Src/input.c
> @@ -223,13 +223,20 @@ shingetchar(void)
>         return STOUC(*shinbufptr++);
>  
>      shinbufreset();
> -    do {
> +    for (;;) {
>         errno = 0;
> -       nread = read(SHIN, shinbuffer, SHINBUFSIZE);
> -    } while (nread < 0 && errno == EINTR);
> -    if (nread <= 0)
> +       nread = read(SHIN, shinbufendptr, 1);
> +       if (nread > 0) {
> +           /* Use line buffering (POSIX requirement) */
> +           if (*shinbufendptr++ == '\n')
> +               break;
> +           if (shinbufendptr == shinbuffer + SHINBUFSIZE)
> +               break;
> +       } else if (nread == 0 || errno != EINTR)
> +           break;
> +    }
> +    if (shinbufendptr == shinbuffer)
>         return -1;
> -    shinbufendptr = shinbuffer + nread;
>      return STOUC(*shinbufptr++);
>  }
>  
> diff --git a/Test/A01grammar.ztst b/Test/A01grammar.ztst
> index 4e39a8f3c..0312fe94e 100644
> --- a/Test/A01grammar.ztst
> +++ b/Test/A01grammar.ztst
> @@ -961,3 +961,12 @@ F:Note that the behaviour of 'exit' inside try-list
> inside a function is unspeci
>  F:This test was written to ensure the behaviour doesn't change silently.
>  F:If this test fails during development, it *might* be appropriate to
> change
>  F:its expectations.
> +
> + (
> +   export VALUE=first
> +   print -l 'echo Value is $VALUE' 'VALUE=second sh' 'echo Value is $VALUE'
> |
> +   $ZTST_testdir/../Src/zsh -f
> + )
> +0:Non-interactive shell command input is line buffered
> +>Value is first
> +>Value is second

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat